Skip to content

Non-API breaking derives for error & transaction types#558

Merged
apoelstra merged 1 commit intorust-bitcoin:masterfrom
BP-WG:fix/error-derives-1
Apr 21, 2021
Merged

Non-API breaking derives for error & transaction types#558
apoelstra merged 1 commit intorust-bitcoin:masterfrom
BP-WG:fix/error-derives-1

Conversation

@dr-orlovsky
Copy link
Copy Markdown
Collaborator

@dr-orlovsky dr-orlovsky commented Jan 30, 2021 •
edited
Loading

This implements first part of #555 recommendations for Error types within rust-bitcoin. This PR is (a) non-API breaking & (b) can be applied before similar PRs in other repos. It will be followed by more PRs that are (1) depend on error type derives from downstream repos & (2) will be API-breaking

PS: depends on rust-bitcoin/rust-bitcoinconsensus#22 merge & new version release

@dr-orlovsky dr-orlovsky requested a review from apoelstra January 30, 2021 13:03
@dr-orlovsky dr-orlovsky mentioned this pull request Jan 30, 2021
Merged
3 tasks
@dr-orlovsky dr-orlovsky added the minor API Change This PR should get a minor version bump label Jan 30, 2021
@dr-orlovsky dr-orlovsky mentioned this pull request Jan 30, 2021
Closed
@dr-orlovsky dr-orlovsky added this to the 0.26.1 milestone Jan 30, 2021
@dr-orlovsky
Copy link
Copy Markdown
Collaborator Author

dr-orlovsky commented Feb 21, 2021

Now with rust-bitcoin/rust-bitcoinconsensus#22 merged this is ready for the review

@dr-orlovsky
Copy link
Copy Markdown
Collaborator Author

dr-orlovsky commented Feb 21, 2021

CI is failing since it requires cargo update to get the newer version of libbitcoinconsensus

@dr-orlovsky dr-orlovsky mentioned this pull request Feb 22, 2021
Merged
@stevenroose
Copy link
Copy Markdown
Collaborator

stevenroose commented Mar 13, 2021

Can you perhaps rebase and remove the dependency-bump changes? They seem unrelated.

@dr-orlovsky
Copy link
Copy Markdown
Collaborator Author

dr-orlovsky commented Apr 6, 2021

@stevenroose done

sgeisler
sgeisler approved these changes Apr 6, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 7fe3c4a

Test log: 
Apr 06 16:29:52.451  INFO testinator: Installing rust toolchain 'nightly'
Apr 06 16:29:56.835  INFO testinator: Installing rust toolchain 'stable'
Apr 06 16:29:57.434  INFO testinator: Installing rust toolchain '1.29.0'
Apr 06 16:29:58.524  INFO testinator: Preparing environment for rust 1.29.0 tests (8 configurations)
Apr 06 16:29:58.524  INFO testinator: Preparing environment for rust nightly tests (9 configurations)
Apr 06 16:29:58.524  INFO testinator: Preparing environment for rust stable tests (8 configurations)
Apr 06 16:32:30.653  INFO testinator: Running rust 1.29.0 tests in /tmp/rust-bitcoin-1.29.0.mCVODhfPtdZH/rust-bitcoin
Apr 06 16:32:30.653 DEBUG testinator: Generating lock file with rust=1.29.0
Apr 06 16:32:30.658  INFO testinator: Running rust nightly tests in /tmp/rust-bitcoin-nightly.l6h15e0SZuO1/rust-bitcoin
Apr 06 16:32:30.660  INFO testinator: Running rust stable tests in /tmp/rust-bitcoin-stable.dtypNEgI1NjJ/rust-bitcoin
Apr 06 16:32:44.273 DEBUG testinator: Pinning cc to 1.0.41
Apr 06 16:32:44.425 DEBUG testinator: Pinning serde to 1.0.98
Apr 06 16:32:44.556 DEBUG testinator: Pinning serde_derive to 1.0.98
Apr 06 16:32:44.697 DEBUG testinator: Pinning byteorder to 1.3.4
Apr 06 16:33:21.107  INFO testinator: Test rust=nightly, features=[secp-recovery] succeeded!
Apr 06 16:33:30.378  INFO testinator: Test rust=stable, features=[secp-recovery] succeeded!
Apr 06 16:34:01.598  INFO testinator: Test rust=1.29.0, features=[secp-recovery] succeeded!
Apr 06 16:34:29.033  INFO testinator: Test rust=nightly, features=[use-serde] succeeded!
Apr 06 16:34:31.180  INFO testinator: Test rust=stable, features=[use-serde] succeeded!
Apr 06 16:34:51.714  INFO testinator: Test rust=nightly, features=[base64] succeeded!
Apr 06 16:34:59.879  INFO testinator: Test rust=stable, features=[base64] succeeded!
Apr 06 16:35:20.562  INFO testinator: Test rust=nightly, features=[secp-lowmemory] succeeded!
Apr 06 16:35:31.647  INFO testinator: Test rust=stable, features=[secp-lowmemory] succeeded!
Apr 06 16:35:41.099  INFO testinator: Test rust=nightly, features=[rand] succeeded!
Apr 06 16:36:08.077  INFO testinator: Test rust=nightly, features=[unstable] succeeded!
Apr 06 16:36:08.995  INFO testinator: Test rust=stable, features=[rand] succeeded!
Apr 06 16:36:11.467  INFO testinator: Test rust=1.29.0, features=[use-serde] succeeded!
Apr 06 16:36:48.337  INFO testinator: Test rust=nightly, features=[bitcoinconsensus] succeeded!
Apr 06 16:36:52.872  INFO testinator: Test rust=stable, features=[bitcoinconsensus] succeeded!
Apr 06 16:37:34.142  INFO testinator: Test rust=1.29.0, features=[base64] succeeded!
Apr 06 16:37:34.414  INFO testinator: Test rust=nightly, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,unstable,bitcoinconsensus] succeeded!                                                                                                  
Apr 06 16:37:46.423  INFO testinator: Test rust=stable, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,bitcoinconsensus] succeeded!                                                                                                            
Apr 06 16:37:56.139  INFO testinator: Test rust=nightly, features=[] succeeded!
Apr 06 16:38:12.423  INFO testinator: Test rust=stable, features=[] succeeded!
Apr 06 16:38:25.280  INFO testinator: Test rust=1.29.0, features=[secp-lowmemory] succeeded!
Apr 06 16:39:13.076  INFO testinator: Test rust=1.29.0, features=[rand] succeeded!
Apr 06 16:40:02.013  INFO testinator: Test rust=1.29.0, features=[bitcoinconsensus] succeeded!
Apr 06 16:40:52.505  INFO testinator: Test rust=1.29.0, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,bitcoinconsensus] succeeded!                                                                                                            
Apr 06 16:41:33.824  INFO testinator: Test rust=1.29.0, features=[] succeeded!
Apr 06 16:41:35.901  INFO testinator: Fuzzing deserialize_script
Apr 06 16:42:42.188  INFO testinator: Successfully fuzzed deserialize_script
Apr 06 16:42:42.188  INFO testinator: Fuzzing uint128_fuzz
Apr 06 16:43:43.146  INFO testinator: Successfully fuzzed uint128_fuzz
Apr 06 16:43:43.146  INFO testinator: Fuzzing deserialize_amount
Apr 06 16:44:44.087  INFO testinator: Successfully fuzzed deserialize_amount
Apr 06 16:44:44.087  INFO testinator: Fuzzing deserialize_transaction
Apr 06 16:45:46.243  INFO testinator: Successfully fuzzed deserialize_transaction
Apr 06 16:45:46.243  INFO testinator: Fuzzing deser_net_msg
Apr 06 16:46:49.069  INFO testinator: Successfully fuzzed deser_net_msg
Apr 06 16:46:49.069  INFO testinator: Fuzzing deserialize_address
Apr 06 16:47:50.234  INFO testinator: Successfully fuzzed deserialize_address
Apr 06 16:47:50.235  INFO testinator: Fuzzing deserialize_block
Apr 06 16:48:52.221  INFO testinator: Successfully fuzzed deserialize_block
Apr 06 16:48:52.221  INFO testinator: Fuzzing outpoint_string
Apr 06 16:49:53.256  INFO testinator: Successfully fuzzed outpoint_string
Apr 06 16:49:53.256  INFO testinator: Fuzzing deserialize_psbt
Apr 06 16:50:55.248  INFO testinator: Successfully fuzzed deserialize_psbt

@sgeisler sgeisler closed this Apr 6, 2021
@sgeisler sgeisler reopened this Apr 6, 2021
@sgeisler
Copy link
Copy Markdown
Contributor

sgeisler commented Apr 6, 2021

Oh, wait, what did you rebase it on? It seems to be missing #583, so CI still fails. I mistook it for a GH Actions bug, but it isn't I think.

sanket1729
sanket1729 reviewed Apr 6, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: The commit message is not consistent. This PR also adds derives to TxIn/TxOut/Transaction.

I also think default is missing for Transaction. We should add a default with Transaction with version 0 or version 2, locktime 0 and empty in/outs. I think this will quite handy in testing.

I am slightly leaning towards zero because it seems a reasonable default and we can auto derive it.

@dr-orlovsky
Copy link
Copy Markdown
Collaborator Author

dr-orlovsky commented Apr 6, 2021

Oh, wait, what did you rebase it on? It seems to be missing #583

Well, it was merged after I did the rebase. But to me CI here shows that all tests have passed successfully

@dr-orlovsky
Copy link
Copy Markdown
Collaborator Author

dr-orlovsky commented Apr 6, 2021

@sanket1729 I'm afraid Default on Transaction may be controversial. Let's try to do it as a separate PR.

@dr-orlovsky dr-orlovsky changed the title Non-API breaking derives for error types Non-API breaking derives for error & transaction types Apr 6, 2021
apoelstra
apoelstra approved these changes Apr 21, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack 9eb0913c2a57e33384d6ace737c225a41b961702

@apoelstra apoelstra merged commit da477f1 into rust-bitcoin:master Apr 21, 2021
@dr-orlovsky dr-orlovsky linked an issue May 4, 2021 that may be closed by this pull request
Define and enforce standard derives #555
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sanket1729 sanket1729 sanket1729 left review comments

@apoelstra apoelstra apoelstra approved these changes

+1 more reviewer

@sgeisler sgeisler sgeisler approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

minor API Change This PR should get a minor version bump

Projects

None yet

Milestone

0.26.1

Development

Successfully merging this pull request may close these issues.

Define and enforce standard derives

5 participants

@dr-orlovsky @stevenroose @sgeisler @apoelstra @sanket1729