PSET integration tests #108

RCasatta · 2021-12-02T16:26:43Z

~~on top of #100, see only 488cdac (MERGED)~~

~~on top of #109 (MERGED)~~

I would like to introduce some PSET integration testing against elements core.
To do so in a easy/maintainable/isolated way, I added the elementsd crate as a dev dependency.
This crate downloads the node binary if a feature is specified (or allow to specify the executable via env var)

I would like to gather early feedback on this approach before implementing the other cases: blinded, issuance, reissueance, pegin.

sanket1729 · 2021-12-03T05:41:10Z

Strong Concept ACK. We do something similar in rust-miniscript. And I would also like to this for elements-miniscript

RCasatta · 2021-12-03T16:15:33Z

Improved a bit, added blinded transaction, I would like to add also issuance, reissuance and pegin.

Not sure what's happening with CI and rust toolchain 1.29, any help is appreciated.

RCasatta · 2021-12-03T16:16:21Z

Cargo.toml


 serde = { version = "1.0", features=["derive"], optional = true }

+# This should be an optional dev-dependency (only needed for integration tests),
+# but dev-dependency cannot be optional, and without optionality older toolchain try to compile it and fails
+elementsd = { version="0.3.0", features=["0_21_0","bitcoind_22_0"], optional = true }


I would like this to be dev-dependency but as written in the comment I am afraid it cannot be...

You mean the elementsd crate doesn't compile with Rust 1.36?

It doesn't, there were some efforts to support it here rust-bitcoin/bitcoind#31 but in the end, I thought it's fine for integration tests to run on stable

Agreed, it's fine for the integration tests to only be on stable

RCasatta · 2021-12-07T09:40:09Z

tests/pset.rs

+        let psbt_base64 = elementsd.wallet_process_psbt(&psbt_base64);
+        assert_eq!(elementsd.expected_next(&psbt_base64), "extractor");
+
+        // TODO this fails because elements decodepsbt is not printing TxOut::asset (PSET_IN_WITNESS_UTXO)


I am using fn psbt_rtt to verify compatibility between PSET implementations rust-elements and elements, but this requires decodepsbt to print everything in the PSET and I think it's not the case as written in the above comment, maybe @stevenroose could confirm/deny

I cannot compare binaries because PSET key-value-pairs fields aren't ordered the same

This is tricky. I don't see any solution. Also, in general, I don't like the idea of testing using decodeRPCs. If I recall correctly even decoderawtransaction does not print all details. But I don't see any other way for doing this :(

Hmm, yeah it's the same problem like "check if this JSON is equal to that". Because order is not the same, but AFAIK shouldn't matter? You could implement cmp::Eq for the pset type and use that? But that'd be a chore..

Oh and yeah it's certainly possible that some of the PSET RPCs don't report everything. I have never seen or used them tbh, so I wouldn't know.

Implementing cmp::Eq on PSET other than being a mess would not accomplish what we are trying to achieve here (PSET implementations compatibility). Suppose for example rust-elements deserialize is skipping a field in the PSET, you wouldn't notice.

Since encoded binary comparison is excluded because the standard is not enforcing field ordering ElementsProject/elements#1059 I can't think of another way to verify compatibility other than relying on decodepsbt printing all fields.

Unfortunately, I think we just need to comment this test. Raise an issue in upstream elements to add this, and add a remainder issue in rust-elements to re-add these once upstream elements fixes the bug.

sanket1729 · 2021-12-08T02:40:27Z

tests/pset.rs

+        let asset_id = AssetId::from_entropy(entropy.clone());
+        assert_eq!(
+            asset_id.to_string(),
+            "78b4920005fa0156ae3779129338bc2707e4d07cf8a0c2593583e3c1da3bb58c"


I think this relies on order on the order prevout, block hashes etc. I think this might easily break in a future elements release.

Same comment for all hard-coded test values that rely on prevouts.

Not sure what's best here:

remove the asserts

add a comment about the possible change in upcoming elements release

I wanted to add something like if cfg!(all(feature = "elementsd/0_21_0", feature = "elementsd/0_18_1_12")), but it looks rust doesn't support this unless we add features like elementsd_0_21_0 = ["elementsd/0_21_0"] which is probably not worthy for this, but it may be useful to have for other integration tests

I don't see what is the problem. The prevout is actually a utxo and it's explicitly used as an input on the createpsbt call, right?

We are (indirectly) asserting that the utxo (which is the first one obtained from listunspent) is a specific utxo with a fixed hard-coded outpoint. That might change in elements release because the order of coins in listunspent is updated (I am not sure if the ordering is guaranteed to be the same across multiple calls of elementsd of the same version).

I am okay adding a comment here if you think there is some value in asserting the fixed asset_id (partly because I am curious to see if this ever breaks :) ). But if this ever breaks in the future, I think we should just remove it.

I just removed the asserts in 682eb8a, they weren't really asserting something in the scope of this PR

sanket1729 · 2021-12-08T04:41:43Z

tests/pset.rs

+        let psbt_base64 = elementsd.wallet_process_psbt(&psbt_base64);
+        assert_eq!(elementsd.expected_next(&psbt_base64), "extractor");
+
+        // TODO this fails because elements decodepsbt is not printing TxOut::asset (PSET_IN_WITNESS_UTXO)


This is tricky. I don't see any solution. Also, in general, I don't like the idea of testing using decodeRPCs. If I recall correctly even decoderawtransaction does not print all details. But I don't see any other way for doing this :(

RCasatta · 2021-12-09T09:55:39Z

I tested with rust 1.36 and it works , I don't see a way to make it compatible with 1.29.

My understanding is that there is consensus to go with 1.36 in the rust-bitcoin community, so I guess it's fine also here to increase MSRV, and this can be merged then

stevenroose · 2021-12-09T13:57:44Z

Can you rebase on master? @sanket1729 can you also review and ack?

RCasatta · 2021-12-09T14:19:38Z

Rebased

stevenroose · 2021-12-09T14:03:13Z

Cargo.toml


 serde = { version = "1.0", features=["derive"], optional = true }

+# This should be an optional dev-dependency (only needed for integration tests),
+# but dev-dependency cannot be optional, and without optionality older toolchain try to compile it and fails
+elementsd = { version="0.3.0", features=["0_21_0","bitcoind_22_0"], optional = true }


You mean the elementsd crate doesn't compile with Rust 1.36?

stevenroose · 2021-12-09T14:05:40Z

Cargo.toml

 ryu = "<1.0.5"
 bincode = "1.3"
-
+base64 = "0.13.0"


It's ok as a dev-dep, but this crate has historically broken MSRV in a minor release. That's why I created base64-compat at the time..

FYI In the past I suggested the base64 rust-bitcoin feature Kixunil/loptos#1 but in that case it was refused because base64 already in deps

stevenroose · 2021-12-09T14:12:46Z

tests/pset.rs

+        fn get_balances(&self) -> Value;
+    }
+
+    #[cfg_attr(feature = "integration", test)]


You can remove all the feature = "integration" here because that's already present on the module.

good point, removed in 682eb8a

stevenroose · 2021-12-09T14:14:31Z

tests/pset.rs

+extern crate elementsd;
+extern crate rand;
+
+#[cfg(all(test, feature = "integration"))]


I'm not sure if it is, but would it maybe be possible to not have this top-level module, but add the cfg on the self using the bang thing, like #![cfg(all(test, feature = "integration"))] at the top? The #![...] thing is short for "on the module this is in" instead of #[...] which is "on the module (or other thing) this is a prefix to".

good idea, added #![cfg...] and removed the inner module

stevenroose · 2021-12-09T14:19:07Z

tests/pset.rs

+        let asset_id = AssetId::from_entropy(entropy.clone());
+        assert_eq!(
+            asset_id.to_string(),
+            "78b4920005fa0156ae3779129338bc2707e4d07cf8a0c2593583e3c1da3bb58c"


I don't see what is the problem. The prevout is actually a utxo and it's explicitly used as an input on the createpsbt call, right?

stevenroose · 2021-12-09T14:45:58Z

tests/pset.rs

+        let psbt_base64 = elementsd.wallet_process_psbt(&psbt_base64);
+        assert_eq!(elementsd.expected_next(&psbt_base64), "extractor");
+
+        // TODO this fails because elements decodepsbt is not printing TxOut::asset (PSET_IN_WITNESS_UTXO)


Hmm, yeah it's the same problem like "check if this JSON is equal to that". Because order is not the same, but AFAIK shouldn't matter? You could implement cmp::Eq for the pset type and use that? But that'd be a chore..

stevenroose · 2021-12-09T14:46:42Z

tests/pset.rs

+        let psbt_base64 = elementsd.wallet_process_psbt(&psbt_base64);
+        assert_eq!(elementsd.expected_next(&psbt_base64), "extractor");
+
+        // TODO this fails because elements decodepsbt is not printing TxOut::asset (PSET_IN_WITNESS_UTXO)


Oh and yeah it's certainly possible that some of the PSET RPCs don't report everything. I have never seen or used them tbh, so I wouldn't know.

stevenroose · 2021-12-09T14:50:00Z

tests/pset.rs

+            }
+            b_bytes[i] = b_bytes[i].wrapping_sub(1);
+        }
+        assert!(tests > 0)


Are you betting that at least one in a thousand a changed byte results in a valid PSET? 😅 Not sure why you'd want to enforce that? You could also just loop for anything between "both at least 1000 times and at least one test performed" or so?

lol, I added during dev to ensure I was passing in the if branch and forgot to remove, removed in 682eb8a

RCasatta · 2021-12-09T17:37:05Z

I think I am going to squash everything into 1 commit after review...

sanket1729

Looks good to me. Left one small comment. Code review ACK. Can merge after squash

sanket1729 · 2021-12-10T04:26:15Z

tests/pset.rs

+        }
+
+        fn analyze_psbt(&self, psbt: &str) -> Value {
+            self.call("decodepsbt", &[psbt.into()])


nit:
This should call to analyzepsbt. I also don't think this is being used. Can also be removed

sanket1729 · 2021-12-10T05:24:10Z

tests/pset.rs

+        let psbt_base64 = elementsd.wallet_process_psbt(&psbt_base64);
+        assert_eq!(elementsd.expected_next(&psbt_base64), "extractor");
+
+        // TODO this fails because elements decodepsbt is not printing TxOut::asset (PSET_IN_WITNESS_UTXO)


Unfortunately, I think we just need to comment this test. Raise an issue in upstream elements to add this, and add a remainder issue in rust-elements to re-add these once upstream elements fixes the bug.

RCasatta · 2021-12-10T09:34:13Z

added dd92f03, 813ad20 and 657246e

then I squashed into one

sanket1729

code review ACK 31dd6bc

sanket1729 · 2021-12-13T11:59:26Z

I don't know how to disable the 1.29 check in the CI. Will wait some time for @stevenroose's ACK before merging

sanket1729 · 2021-12-15T14:57:27Z

Was about to merge this using bitcoin core merge script . But it looks like I need admin access/not just write access. https://stackoverflow.com/questions/52589285/cannot-push-on-github-suddently

RCasatta force-pushed the integration branch 5 times, most recently from 343aca5 to e78c873 Compare December 3, 2021 16:09

RCasatta commented Dec 3, 2021

View reviewed changes

RCasatta marked this pull request as ready for review December 7, 2021 09:35

RCasatta commented Dec 7, 2021

View reviewed changes

sanket1729 reviewed Dec 8, 2021

View reviewed changes

RCasatta force-pushed the integration branch from b45df06 to b72cb7e Compare December 9, 2021 13:02

RCasatta force-pushed the integration branch from b72cb7e to c901aac Compare December 9, 2021 14:17

stevenroose reviewed Dec 9, 2021

View reviewed changes

sanket1729 reviewed Dec 10, 2021

View reviewed changes

This was referenced Dec 10, 2021

PSBT decodepsbt does not print asset in PSET_IN_WITNESS_UTXO ElementsProject/elements#1080

Open

Remove #[ignore] from integration::tx_pegin test once upstream elements fixes decodepsbt for PSBT_WITNESS_UTXO #110

Open

PSET integration test with elements_core.

31dd6bc

RCasatta force-pushed the integration branch from 657246e to 31dd6bc Compare December 10, 2021 09:31

sanket1729 approved these changes Dec 13, 2021

View reviewed changes

RCasatta merged commit b44b0bb into ElementsProject:master Dec 15, 2021

PSET integration tests #108

PSET integration tests #108

Uh oh!

Conversation

RCasatta commented Dec 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanket1729 commented Dec 3, 2021

Uh oh!

RCasatta commented Dec 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RCasatta Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RCasatta commented Dec 9, 2021

Uh oh!

stevenroose commented Dec 9, 2021

Uh oh!

RCasatta commented Dec 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RCasatta commented Dec 9, 2021

Uh oh!

sanket1729 left a comment

RCasatta commented Dec 2, 2021 •

edited

Loading

RCasatta Dec 7, 2021 •

edited

Loading

sanket1729 commented Dec 13, 2021 •

edited

Loading

sanket1729 commented Dec 15, 2021 •

edited

Loading