upgrade to bitcoin 0.32

[RCasatta]

#208

    fn consensus_encode<W: bitcoin::io::Write + ?Sized>(&self, e: W) -> Result<usize, crate::encode::Error> {}
    fn consensus_decode<R: bitcoin::io::BufRead + ?Sized>(reader: &mut R) -> Result<Self, crate::encode::Error> {}

We went for not depending on bitcoin::Encodable instead

[apoelstra]

The policy in this crate is "do whatever works" to get Encodable/Decodable working. Properly speaking, we should have our own io dependency and copy all the clever nostd stuff that rust-bitcoin does. In practice this crate requires std and has direct references to std/alloc stuff everywhere. So we wind up with "performative" nostd stuff like gating our functions on bitcoin::io::Write instead of std::io::Write even though the crate won't actually work without std.

So yes, your proposal sounds good to me.

If it turns out to be unusable downstream for some reason then we can iterate a bit.

[apoelstra]

For readers who aren't familiar with the gory details of this codebase,

• We have our own elements::Encodable/Decodable traits which are copies of the ones in bitcoin because bitcoin took away some impl that we needed (maybe on &[u8]?)
• However, because we directly deal with bitcoin types in this crate (e.g. in pegin witnesses where we have bitcoin::BlockHashes and merkle proofs and what not), we need elements::Encodable to be implemented on bitcoin types, in terms of bitcoin::Encodable.
• In bitcoin 0.32 we changed bitcoin::Encodable to gate on our bespoke io::Write trait instead of the std::io::Write trait.
• We did not put a blanket bitcoin::io::Write impl on T: std::io::Write even though this is what users expect and need, because the Rust language is stupid and limiting and prevents us from doing so
• Therefore, in order for us to implement elements::Encodable as a passthrough to bitcoin::Encodable, we need to copy the bespoke bounds on bitcoin::Encodable onto elements::Encodable.

That last bullet point isn't quite true: alternately, we could drop the impl_upstream! macro and stop implementing elements::Encodable directly in terms of bitcoin::Encodable. In fact most of these impls would be one-liners (if we had a utility function to serialize a &[u8] as a length prefix I think they all would be, except for btcenc::VarInt which would be 4 or 5 lines) (though VarInt we should really drop in favor of utility functions on some sort of WriteExt/ReadExt traits, as we are doing upstream rust-bitcoin/rust-bitcoin#2931) .

Actually maybe @RCasatta you want to try that? Replacing impl_upstream and getting rid of the btcenc import in src/encode.rs entirely? When I started typing I thought it would be a giant pain in the ass, but actually I think it's fairly straightforward, and we could even do it in a PR separately to the upgrade.

[RCasatta]

Actually maybe @RCasatta you want to try that?

I am going to try

[RCasatta]

I did some initial work here 8ce9c35

But we still need something similar to impl_upstream because of the bitcoin types in pset right?

[apoelstra]

PSET has its own Deserialize/Serialize trait that mostly passes through to Encodable/Decodable. So there should be minimal changes required to PSET once you have Encodable/Decodable working.

[RCasatta]

still needing something along the lines of ElementsProject/elements-miniscript#88

[RCasatta]

Integration testing is now passing but testing locally with elements Elements Core version v23.2.1 I hit an error:

[nix-shell:~/git/rust-elements/elementsd-tests]$ elementsd --version
Elements Core version v23.2.1
[nix-shell:~/git/rust-elements/elementsd-tests]$ cargo test -- tx_issuance
    Finished test [unoptimized + debuginfo] target(s) in 0.16s
     Running unittests src/lib.rs (/home/casatta/git/rust-elements/target/debug/deps/elementsd_tests-534535ebc6e90ec9)

running 1 test
test pset::tx_issuance ... FAILED

failures:

---- pset::tx_issuance stdout ----
thread 'pset::tx_issuance' panicked at elementsd-tests/src/lib.rs:37:23:
error JSON-RPC error: RPC error response: RpcError { code: -4, message: "Unable to create an asset surjection proof", data: None } while calling walletprocesspsbt with [String("cHNldP8BAgQCAAAAAQMEAAAAAAEEAQEBBQEEAfsEAgAAAAABDiAIe20YE7qmV3wszCrvpMzUqdI8fWWYRhP0/w9AOl/MkQEPBAAAAAABEAT/////B/wEcHNldAAIAOh2SBcAAAAH/ARwc2V0CggA4fUFAAAAAAABAwgA6HZIFwAAAAEEFgAUIG3HsMlrrmBZ2Z/fkew6jvzw8cQH/ARwc2V0AiCMtTvaweODNVnCoPh80OQHJ7w4kxJ5N65WAfoFAJK0eAf8BHBzZXQGIQNwqIVVbZjBP3WbfM2Q8rAf+UgZ18FXTFz63sjjROxbSwf8BHBzZXQIBAAAAAAAAQMIAOH1BQAAAAABBBYAFOQA+B8RK4Xe3KZuDBdXMUH8TdKBB/wEcHNldAIg+wI9yyBNBZ8YSEWrxZOS/GiW+fnJqyUNM5YkNyoWVXYH/ARwc2V0BiECsxS+LIBwaCdgZNH/FvEcvBsFUfRkMB7K4CviLIwcRUsH/ARwc2V0CAQAAAAAAAEDCIDeknwAAAAAAQQWABTCiuEQ1wJ5ohJXRzHqxwXhaEAAMwf8BHBzZXQCICWyUQcOKcoZBDzzPM1zJOLdqwPsxK4LXnfE/A5c9slaB/wEcHNldAYhAweZZvsyOP39XpkGG0xIH5QZsB2o3GC4rAhnpG4dF7ffB/wEcHNldAgEAAAAAAABAwiAlpgAAAAAAAEEAAf8BHBzZXQCICWyUQcOKcoZBDzzPM1zJOLdqwPsxK4LXnfE/A5c9slaAA==")]
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[delta1]

reproduced the asset surjection proof error with elementsd v22.1.1 and v23.2.2

it does not happen with elementsd v0.21.0.3

[LeoComandini]

@RCasatta

Making the issuance unblinded fixes the issue

diff --git a/elementsd-tests/src/pset.rs b/elementsd-tests/src/pset.rs
index dd2d92b..6c475b0 100644
--- a/elementsd-tests/src/pset.rs
+++ b/elementsd-tests/src/pset.rs
@@ -55,12 +55,12 @@ fn tx_issuance() {
     let contract_hash = ContractHash::from_byte_array([0u8; 32]);
     let entropy = AssetId::generate_asset_entropy(prevout, contract_hash);
     let asset_id = AssetId::from_entropy(entropy.clone());
-    let reissuance_id = AssetId::reissuance_token_from_entropy(entropy, true);
+    let reissuance_id = AssetId::reissuance_token_from_entropy(entropy, false);
 
     let value = elementsd.call(
             "createpsbt",
             &[
-                json!([{ "txid": prevout.txid.to_string(), "vout": prevout.vout, "issuance_amount": 1000, "issuance_tokens": 1}]),
+                json!([{ "txid": prevout.txid.to_string(), "vout": prevout.vout, "issuance_amount": 1000, "issuance_tokens": 1, "blind_reissuance": false}]),
                 json!([
                     {address_asset: "1000", "asset": asset_id.to_string(), "blinder_index": 0},
                     {address_reissuance: "1", "asset": reissuance_id.to_string(), "blinder_index": 0},

In the end this is a elementsd issue: PSET is created by elementsd and the walletprocesspsbt fails.
IMHO this should be addressed in the elements repo, or if we want to do it here at least in a separate PR.

[LeoComandini]

@RCasatta

The diff does not work with elements 21... anyway I think this is a elementsd issue, so we can't do much on the rust-elements side.
Raised #214 for tracking

[LeoComandini]

utACK 61091a7, code review

[apoelstra]

Can you squash the first two commits together? The test crate doesn't compile with the first one due to version mismatches.

[RCasatta]

First two commits squashed

[apoelstra]

ACK f889525 successfully ran local tests

[apoelstra]

Edited PR description to remove crossed out text, since it didn't seem essential and because it @-referenced me, which the merge script did not like.