-
Notifications
You must be signed in to change notification settings - Fork 49
[types] Add missing return fields for gettxoutsetinfo RPC
#475
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
60cdd1a to
87826eb
Compare
87826eb to
d776988
Compare
jamillambert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK d776988
types/src/model/blockchain.rs
Outdated
| /// The total amount. | ||
| pub total_amount: Amount, | ||
| /// The serialized hash (only present if 'muhash' hash_type is chosen). | ||
| pub muhash: Option<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which hash algo is used? We can use a concrete type if we know the algo. Is it SHA256?
lolz, I see we have a FIXME already in the code asking this same question for hash_serialized_2.
So can you either add a FIXME to this line and hash_serialized_3 or work out an answer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The algorithm is MuHash3072, which from what I understand is like a bucket of hashes. There are libraries that could handle MuHash such as google/rust-multihash (discontinued) or kaspa_muhash. I don't really know how to work with MuHash, or want to include a unknown library in types crate, I decided to put a FIXME there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, raised: rust-bitcoin/rust-bitcoin#5589
types/src/model/blockchain.rs
Outdated
|
|
||
| /// Detailed block-level info. Part of `gettxoutsetinfo`. | ||
| #[derive(Clone, Debug, PartialEq, Deserialize, Serialize)] | ||
| pub struct BlockInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should be called GetTxOutSetInfoBlockInfo? Since we only have one namespace at the model (and also version specific eg types::v30) level this takes up a name that is quite ambiguous.
Precedent is set by ScanTxOutSetFoo for method scantxoutset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, my bad this was an oversight. Fixed
types/src/model/blockchain.rs
Outdated
|
|
||
| /// Categories of unspendable amounts. Part of `gettxoutsetinfo`. | ||
| #[derive(Clone, Debug, PartialEq, Deserialize, Serialize)] | ||
| pub struct Unspendables { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps same comment as on BlockInfo although 'unspendables' seems less common?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the prefix, went ahead and also added it here.
| UnspendablesUnclaimedRewards(amount::ParseAmountError), | ||
| /// Conversion of the `total_unspendable_amount` field failed. | ||
| TotalUnspendableAmount(amount::ParseAmountError), | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this error stuff. Including the variants for optional stuff. @jamillambert did we do this before or did we use a nested error type? I can't remember.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In v26 there is an example: SubmitPackage (response for RPC) has optional field fees nested within tx_result list, called SubmitPackageTxResultFees, which has errors defined for all its fields too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aight, good enough for me. Thanks
types/src/v26/blockchain/into.rs
Outdated
| Some(b) => { | ||
| let prevout_spent = | ||
| Amount::from_btc(b.prevout_spent).map_err(E::PrevoutSpent)?; | ||
| let coinbase = Amount::from_btc(b.coinbase).map_err(E::Coinbase)?; | ||
| let new_outputs_ex_coinbase = Amount::from_btc(b.new_outputs_ex_coinbase) | ||
| .map_err(E::NewOutputsExCoinbase)?; | ||
| let unspendable = Amount::from_btc(b.unspendable).map_err(E::Unspendable)?; | ||
| let unspendables = model::Unspendables { | ||
| genesis_block: Amount::from_btc(b.unspendables.genesis_block) | ||
| .map_err(E::UnspendablesGenesisBlock)?, | ||
| bip30: Amount::from_btc(b.unspendables.bip30) | ||
| .map_err(E::UnspendablesBip30)?, | ||
| scripts: Amount::from_btc(b.unspendables.scripts) | ||
| .map_err(E::UnspendablesScripts)?, | ||
| unclaimed_rewards: Amount::from_btc(b.unspendables.unclaimed_rewards) | ||
| .map_err(E::UnspendablesUnclaimedRewards)?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put all the conversion logic up above the return statement please? I know is subjective but that is how we have done in everywhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work man, this was a big one.
Reviewed: d776988
d776988 to
b10f6d0
Compare
|
You can fix formatting by running |
The RPC has missing fields which are enabled only when `coinstatsindex` or muhash hash type is used. Argument list is also added to header coment.
b10f6d0 to
01f13a9
Compare
tcharding
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 01f13a9
The RPC has missing fields which are enabled only when
coinstatsindexoption is called or muhash hash type is used. Argument list is also added to header comment.Partially fixes issue #474.