PSBT base64 (de)serialization with Display & FromStr #557
Conversation
There was a problem hiding this comment.
I left some nits, more descriptive errors are worth an API break imo.
EDIT: I just saw that a6399e7 is its own commit 😄 is it so important to you to keep the API stable to get into a point release? Seems hacky and we should definitely open an issue on merge or revert right away after the point release.
src/util/psbt/mod.rs
Outdated
| impl Display for PartiallySignedTransaction { | ||
| fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { | ||
| use consensus::encode::serialize; | ||
| f.write_str(&::base64::encode(&serialize(self))) |
There was a problem hiding this comment.
This allocation is avoidable by using base64::display::Base64Display. I'm not sure if it's worth the inconsistency with decoding, but wanted to point out that option.
|
@sgeisler thank you for the review! The reason for minor version is the following: 0.27 will take months to be published and will do a large-scale refactoring of the repo; while this thing is needed to be published within days/week, if possible. |
|
As long as you take ownership of reverting the commit I'm ok with it, I'd just be unhappy with it staying that way and we don't exactly have a track record of being super organized as an org 😄 |
|
Improved display with your suggestion - and yes, I will create revert PR right after this one will be merged |
There was a problem hiding this comment.
tACK b407721.
Left a minor nit, it will require some changes again in the revertme commit which are anyways going to be reverted. But I think that this is the right thing to do. I am not bent on it, as that is something related to repo in general and not something about this PR.
We should also open an issue with milestone 0.27 so that we don't forget to revert the last commit.
src/util/psbt/mod.rs
Outdated
|
|
||
| #[cfg(feature = "base64")] { | ||
| let psbt2 = PartiallySignedTransaction::from_str("cHNidP8BAHUCAAAAASaBcTce3/KF6Tet7qSze3gADAVmy7OtZGQXE8pCFxv2AAAAAAD+////AtPf9QUAAAAAGXapFNDFmQPFusKGh2DpD9UhpGZap2UgiKwA4fUFAAAAABepFDVF5uM7gyxHBQ8k0+65PJwDlIvHh7MuEwAAAQD9pQEBAAAAAAECiaPHHqtNIOA3G7ukzGmPopXJRjr6Ljl/hTPMti+VZ+UBAAAAFxYAFL4Y0VKpsBIDna89p95PUzSe7LmF/////4b4qkOnHf8USIk6UwpyN+9rRgi7st0tAXHmOuxqSJC0AQAAABcWABT+Pp7xp0XpdNkCxDVZQ6vLNL1TU/////8CAMLrCwAAAAAZdqkUhc/xCX/Z4Ai7NK9wnGIZeziXikiIrHL++E4sAAAAF6kUM5cluiHv1irHU6m80GfWx6ajnQWHAkcwRAIgJxK+IuAnDzlPVoMR3HyppolwuAJf3TskAinwf4pfOiQCIAGLONfc0xTnNMkna9b7QPZzMlvEuqFEyADS8vAtsnZcASED0uFWdJQbrUqZY3LLh+GFbTZSYG2YVi/jnF6efkE/IQUCSDBFAiEA0SuFLYXc2WHS9fSrZgZU327tzHlMDDPOXMMJ/7X85Y0CIGczio4OFyXBl/saiK9Z9R5E5CVbIBZ8hoQDHAXR8lkqASECI7cr7vCWXRC+B3jv7NYfysb3mk6haTkzgHNEZPhPKrMAAAAAAA=="); | ||
| assert!(psbt.is_err()); |
There was a problem hiding this comment.
I used to is_err type checks before, but am shifting more towards asserting the error string equality.
I realize that most of the repo probably has is_err checks everywhere, but we can at least make sure that incoming changes assert the correct variant.
There was a problem hiding this comment.
I am using #[should_fail(expect="...")] in my projects, but would not recommend it since tests break each time you proof/change description of the error
There was a problem hiding this comment.
I think that tests should fail if we change the description
There was a problem hiding this comment.
Done as a separate commit, also improving error testing for previously existing tests: fc006bb
There was a problem hiding this comment.
I think that tests should fail if we change the description
I don't think this should be an explicit goal here. Since we can test for error-equality now that most of the derive-PRs are merged we should do that instead. If you feel strongly about error messages being tested that is probably better done in a separate test.
|
@sanket1729 done with #564. I propose to merge this for now and discuss the best way of handling errors in that issue |
|
@dr-orlovsky, I prefer not to merge unless I get a sign of approval from @apoelstra's powerful machine which runs more powerful checks than the CI, checks that every commit builds on the respective rustc versions that we care about. Maybe this is turning up too much work on @apoelstra and we need to find a better way. I am awaiting Andrew's comment before I merge this, anyways we need him for publishing the minor release. |
|
@sanket1729 Are there any practical limits for open source projects on GH actions? I think we could test a lot more if we wanted to, possibly gated by an initial smaller stage to not burn CPU needlessly. EDIT: reading about it: 20 concurrent jobs per account (are orgs accounts or does every one of us have 20 parallel workers?) + 72h execution time per workflow should be more than enough for our purposes. |
|
@sgeisler, I think it also checks every single commit builds/tests on different rust versions which is I only do for the stable rust versions. But we should see if we get per commit build/test checks in the CI script. Strangely enough, I can't easily find any project with CI that checks on every commit. |
|
I tested b407721 quite extensively now (full feature matrix stable+nightly and multiple fuzzing rounds), I think we can just merge except if we want @apoelstra's ok regarding the release-and-revert strategy. |
There was a problem hiding this comment.
@darosior found some mistakes in the unit tests, so they should be fixed.
otherwise utACK
sgeisler
dismissed
their stale review
We need to solve the error problem in a satisfactory way first.
|
After a lot of confusion with the latest revert commit preventing API break, I have restructured the PR in this way:
|
dr-orlovsky
requested review from
apoelstra,
sanket1729,
sgeisler and
stevenroose
|
Improved after discussion in #574 (comment) |
|
Needs a fix for 1.29.0, but I like the error solution. |
There was a problem hiding this comment.
tACK 7b7b73f
Test log:
pr-557-head-7b7b73f6d2653d42be5bd63aa01a07680a7539af.log:
Feb 22 13:36:10.555 INFO testinator: Installing rust toolchain 'nightly'
Feb 22 13:36:10.943 INFO testinator: Installing rust toolchain 'stable'
Feb 22 13:36:11.191 INFO testinator: Installing rust toolchain '1.29.0'
Feb 22 13:36:11.383 INFO testinator: Preparing environment for rust stable tests (8 configurations)
Feb 22 13:36:11.383 INFO testinator: Preparing environment for rust 1.29.0 tests (8 configurations)
Feb 22 13:36:11.384 INFO testinator: Preparing environment for rust nightly tests (9 configurations)
Feb 22 13:37:25.931 INFO testinator: Running rust nightly tests in /tmp/rust-bitcoin-nightly.DBBudwObJKaQ/rust-bitcoin
Feb 22 13:37:25.934 INFO testinator: Running rust stable tests in /tmp/rust-bitcoin-stable.qCyzHlx68R9I/rust-bitcoin
Feb 22 13:37:25.937 INFO testinator: Running rust 1.29.0 tests in /tmp/rust-bitcoin-1.29.0.AlaHDIqQLlAf/rust-bitcoin
Feb 22 13:37:25.937 DEBUG testinator: Generating lock file with rust=1.29.0
Feb 22 13:37:27.893 DEBUG testinator: Pinning cc to 1.0.41
Feb 22 13:37:28.062 DEBUG testinator: Pinning serde to 1.0.98
Feb 22 13:37:28.278 DEBUG testinator: Pinning serde_derive to 1.0.98
Feb 22 13:37:28.628 DEBUG testinator: Pinning byteorder to 1.3.4
Feb 22 13:38:23.294 INFO testinator: Test rust=nightly, features=[secp-recovery] succeeded!
Feb 22 13:38:25.660 INFO testinator: Test rust=stable, features=[secp-recovery] succeeded!
Feb 22 13:39:01.637 INFO testinator: Test rust=1.29.0, features=[secp-recovery] succeeded!
Feb 22 13:39:05.752 INFO testinator: Test rust=nightly, features=[use-serde] succeeded!
Feb 22 13:39:28.007 INFO testinator: Test rust=stable, features=[use-serde] succeeded!
Feb 22 13:39:34.832 INFO testinator: Test rust=nightly, features=[base64] succeeded!
Feb 22 13:39:57.354 INFO testinator: Test rust=stable, features=[base64] succeeded!
Feb 22 13:39:58.577 INFO testinator: Test rust=nightly, features=[secp-lowmemory] succeeded!
Feb 22 13:40:21.213 INFO testinator: Test rust=nightly, features=[rand] succeeded!
Feb 22 13:40:28.537 INFO testinator: Test rust=stable, features=[secp-lowmemory] succeeded!
Feb 22 13:40:47.660 INFO testinator: Test rust=nightly, features=[unstable] succeeded!
Feb 22 13:41:07.994 INFO testinator: Test rust=stable, features=[rand] succeeded!
Feb 22 13:41:08.000 INFO testinator: Test rust=1.29.0, features=[use-serde] succeeded!
Feb 22 13:41:35.112 INFO testinator: Test rust=nightly, features=[bitcoinconsensus] succeeded!
Feb 22 13:41:44.723 INFO testinator: Test rust=stable, features=[bitcoinconsensus] succeeded!
Feb 22 13:42:09.568 INFO testinator: Test rust=nightly, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,unstable,bitcoinconsensus] succeeded!
Feb 22 13:42:11.081 INFO testinator: Test rust=1.29.0, features=[base64] succeeded!
Feb 22 13:42:22.465 INFO testinator: Test rust=stable, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,bitcoinconsensus] succeeded!
Feb 22 13:42:34.005 INFO testinator: Test rust=nightly, features=[] succeeded!
Feb 22 13:42:50.333 INFO testinator: Test rust=stable, features=[] succeeded!
Feb 22 13:43:09.905 INFO testinator: Test rust=1.29.0, features=[secp-lowmemory] succeeded!
Feb 22 13:43:50.721 INFO testinator: Test rust=1.29.0, features=[rand] succeeded!
Feb 22 13:44:38.477 INFO testinator: Test rust=1.29.0, features=[bitcoinconsensus] succeeded!
Feb 22 13:45:26.189 INFO testinator: Test rust=1.29.0, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,bitcoinconsensus] succeeded!
Feb 22 13:46:12.448 INFO testinator: Test rust=1.29.0, features=[] succeeded!
Feb 22 13:46:14.303 INFO testinator: Fuzzing deserialize_script
Feb 22 13:47:15.099 INFO testinator: Successfully fuzzed deserialize_script
Feb 22 13:47:15.100 INFO testinator: Fuzzing uint128_fuzz
Feb 22 13:48:16.205 INFO testinator: Successfully fuzzed uint128_fuzz
Feb 22 13:48:16.205 INFO testinator: Fuzzing deserialize_amount
Feb 22 13:49:17.119 INFO testinator: Successfully fuzzed deserialize_amount
Feb 22 13:49:17.119 INFO testinator: Fuzzing deserialize_transaction
Feb 22 13:50:18.245 INFO testinator: Successfully fuzzed deserialize_transaction
Feb 22 13:50:18.245 INFO testinator: Fuzzing deser_net_msg
Feb 22 13:51:19.223 INFO testinator: Successfully fuzzed deser_net_msg
Feb 22 13:51:19.223 INFO testinator: Fuzzing deserialize_address
Feb 22 13:52:20.136 INFO testinator: Successfully fuzzed deserialize_address
Feb 22 13:52:20.136 INFO testinator: Fuzzing deserialize_block
Feb 22 13:53:21.067 INFO testinator: Successfully fuzzed deserialize_block
Feb 22 13:53:21.067 INFO testinator: Fuzzing outpoint_string
Feb 22 13:54:22.170 INFO testinator: Successfully fuzzed outpoint_string
Feb 22 13:54:22.170 INFO testinator: Fuzzing deserialize_psbt
Feb 22 13:55:23.141 INFO testinator: Successfully fuzzed deserialize_psbt
@sgeisler we should use the issues feature more and have a release board or so at least. But well I'm not really the one to speak, I have been really bad keeping up with review lately. |
Was quite surprised that the lib can't serialize and read Base64 PSBTs as required by BIP-174 (while having base64 as a dependency). Since this is a way of putting PSBT into a string as defined by the standard I used it as a way for implementing
FromStr/Display.Seems like an API break since we need to add onepsbt::Errorvariant (feature-gated, though).Provided one other commit a6399e7 that makes the PR non-API-breaking, suitable for0.26.1(really like to have base64 encoding ASAP). After the minor release that commit can be reverted.