Finish off support for Bitcoin Core v31.0#615
Conversation
|
Latest force push uses enums for getpeerinfo types, maintaining full v31 docs in corepc (before, it simply discarded relevant messages from Bitcoin Core documentation). EDIT: below i retriggered CI |
|
Looks pretty good man. To create all the new types you just copied the one from the latest version and added the fields, right? I.e for |
|
Reviewed: 1d584d8 |
Yes. Everything that didn't need to change hasn't changed. Diffs only in new/modified fields or the corresponding docs. All "institutional memory" is kept. |
jamillambert
left a comment
jamillambert
left a comment
There was a problem hiding this comment.
I've had a look through and it all looks good so far. There is a lot here at so will need to have another look tomorrow.
|
The first commit doesn't build. Can you get the PR into a state where I can review patch by patch and on each I can run |
satsfy
force-pushed
the
v31
branch
2 times, most recently
from
508d5cf to
aad9a24
Compare
|
Force push fixed the bissectability of every commit. EDIT: no, a90eb31 is broken. |
satsfy
force-pushed
the
v31
branch
2 times, most recently
from
508d5cf to
150016f
Compare
Add getmempoolcluster and getmempoolfeeratediagram RPCs. Update getmempoolentry, getrawmempool, getmempoolancestors, getmempooldescendants and getmempoolinfo with new field chunk_weight, chunk_fee and cluster_limit fields. Back-fill optional fields as None for earlier versions.
RPC getdeploymentinfo gets script_flags field. This commit adds an Enum with the possible flags.
RPC gettxspendingprevout adds spending_tx and block_hash fields when return_spending_tx is set.
RPC getblock gets new field coinbase_tx. Commit backfills None for earlier versions.
Deny unknown fields. Use i64 for peer timestamps because time can be emmited as a negative.
getblockfrompeer, disconnectnode, getnetworkinfo, getblocktemplate and waitfornewblock each carried a a note that said the fields since v30, but their modelled return types match v30.
Despite getmempoolfeeratediagram being a new RPC, it is hidden. It is not included in the rustdoc method table. Remove SetTxFee import because the RPC has been deleted but the re-export had been kept.
|
Latest force push:
|
Drop the v30_and_below gates from tests covering RPCs unchanged in v31.
|
Clean up accidental garbage file |
jamillambert
left a comment
jamillambert
left a comment
There was a problem hiding this comment.
ACK daf4571
Nice one
| #[cfg(not(feature = "v30_and_below"))] | ||
| let _ = (logging.kernel, logging.privatebroadcast); |
There was a problem hiding this comment.
What is the intention of this line? It doesn't prove anything, unless I'm missing something.
There was a problem hiding this comment.
I see my bad, the test is pointless. I was trying to enforce field presence so that I get errors like this:
❯ cargo test --features 31_0,download
error[E0609]: no field `kernel` on type `bitcoind::vtype::Logging`
--> tests/control.rs:34:22
34 | let _ = (logging.kernel, logging.privatebroadcast);
| ^^^^^^ unknown field But the line let _: Logging = node.client.logging().unwrap(); already does it. Removing.
|
|
||
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| pub use super::{GetWalletInfoError, GetWalletInfoScanning, LastProcessedBlock}; |
There was a problem hiding this comment.
This works but it will break if we later change these types. Better to import them from v30 I rekon.
| @@ -0,0 +1,57 @@ | |||
| // SPDX-License-Identifier: CC0-1.0 | |||
|
|
|||
| use super::{GetWalletInfo, GetWalletInfoError, GetWalletInfoScanning}; | |||
There was a problem hiding this comment.
While this works I'd prefer a new error type otherwise the
/// Conversion of the `pay_tx_fee` field failed.
PayTxFee(ParseAmountError),variant is never used.
| GetBlockVerboseOneError, GetBlockVerboseThreeError, GetBlockVerboseTwoError, | ||
| GetDeploymentInfoError, GetMempoolInfoError, |
There was a problem hiding this comment.
I didn't look closely but like mentioned elsewhere if these have unused variants then we want new error types. If not import directly from the version specific module.
There was a problem hiding this comment.
FTR I'm not totally sure we have this correct in other places in the crate. I may have been slack reviewing before and let them in. Thats the nature of this crate unfortunately, PRs are often massive.
| /// The coinbase transaction version. | ||
| pub version: i32, | ||
| /// The coinbase transaction's locktime. | ||
| pub locktime: u32, | ||
| /// The coinbase input's sequence number (nSequence). | ||
| pub sequence: u32, |
There was a problem hiding this comment.
These three can all be concrete types from rusti-bitcoin.
| /// The coinbase input's script. | ||
| pub coinbase: String, | ||
| /// The coinbase input's first (and only) witness stack element, if present. | ||
| pub witness: Option<String>, |
There was a problem hiding this comment.
Maybe this one too, didn't look closely.
| /// | ||
| /// Introduced in Bitcoin Core v31. | ||
| #[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize)] | ||
| pub struct CoinbaseTransaction { |
There was a problem hiding this comment.
Can you put this below where its used please. I always favour this style based on the idea to put the most important stuff first in a file.
There was a problem hiding this comment.
Oh I see in other places we don't do this. If a type is used in a bunch of places we do tend to throw it up top of the file. Just do what you like here.
| impl CoinbaseTransaction { | ||
| /// Converts version specific type to a version nonspecific, more strongly typed type. | ||
| pub fn into_model(self) -> model::CoinbaseTransaction { | ||
| model::CoinbaseTransaction { | ||
| version: self.version, | ||
| locktime: self.locktime, | ||
| sequence: self.sequence, | ||
| coinbase: self.coinbase, | ||
| witness: self.witness, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
For your learning, this code hints that the type is wrong. There is not need to have an into_model function for a type that does no type conversions.
| pub fn get_tx_spending_prevout( | ||
| &self, | ||
| outputs: &[bitcoin::OutPoint], | ||
| mempool_only: bool, | ||
| return_spending_tx: bool, | ||
| ) -> Result<GetTxSpendingPrevout> { |
There was a problem hiding this comment.
Typically I'd try to avoid boolean args like this. Is there some precedence in the repo already for doing this?
Another option would be to just always pass these args in. I did not look too closely though.
There was a problem hiding this comment.
Oh, so this is entirely optional. I'm removing it.
|
I'm going to merge this as is because its big. Can you see to review suggestions as a follow up please? |
|
After the follow up PR we can do a release for this stuff. Well done man! |
78a7762 client: drop options from gettxspendingprevout (satsfy (Renato Britto)) 4d998ea types: use rust-bitcoin types in the coinbase tx model (satsfy (Renato Britto)) a68aedd types: add a v31 getwalletinfo error type (satsfy (Renato Britto)) 029fbf5 test: simplify control logging test (satsfy (Renato Britto)) Pull request description: These are follow-ups to the v31.0 support work, addressing [review comments](#615) left after it was merged. - `CoinbaseTransaction` now uses concrete rust-bitcoin types instead of primitives. v31 gains new error types to carry the `coinbase_tx` conversion error. - Add `GetWalletInfoError` because 31 no longer returns `paytxfee`. `GetWalletInfoScanning` and `LastProcessedBlock` types it reuses are imported from v30 directly rather than through the v31 re-export. - Redundant `logging` test removed. - Avoid boolean arguments on `get_tx_spending_prevout` by drops `mempool_only` and `return_spending_tx`. ACKs for top commit: tcharding: ACK 78a7762 Tree-SHA512: 155745cb51f8eda9cd14a55e18f55442080c0604a650bb98a7e0eca981ec8287b3908b85d07489d403a50f64d5e154cf08463152b4d6e868f46806ad2a391091
4 participants
Closes #599
Completes v31 support across the client, types, and integration tests. Makes v31 the default version.
Fixed:
inv_to_sendandlast_inv_sequence, and madestartingheightoptional now that it is only returned under-deprecatedrpc=startingheight.kernelandprivatebroadcastcategories.paytxfee.script_flagsfield.spendingtxandblockhashfields, plus added support for the newmempool_onlyandreturn_spending_txarguments.coinbase_txobject returned at verbosity 1, 2 and 3.Added:
getmempoolclusterandgetmempoolfeeratediagramRPCs, thechunk_weightandchunk_feefields ongetmempoolentry,getrawmempool,getmempoolancestorsandgetmempooldescendants, and thecluster_limitandoptimalfields ongetmempoolinfo.getprivatebroadcastinfoandabortprivatebroadcastRPCs.LLM usage: Claude helped on generating tests for new RPCs, verifying things over and over again, attempting to call every RPC, spotting additional unfixed things, and reviewing every change the PR.
When appropriate, comments have been checked to match v31 documentation exactly.