[Rpcsaga] add blockhash to gettxoutproof #472
Conversation
9ff7536 to
9a8bed0
Compare
|
Applied @Davidson-Souza requests and rewrote the commit message, now appears to better explain whats being done here. Refer to the PR description to read the last commit message |
9a8bed0 to
a336c21
Compare
|
Applied @Davidson-Souza s suggestions. (#472 (comment)) |
florestad/src/json_rpc/blockchain.rs
Outdated
| &self, | ||
| tx_ids: &[Txid], | ||
| blockhash: Option<BlockHash>, | ||
| ) -> Result<Vec<Vec<String>>, RpcError> { |
There was a problem hiding this comment.
From Core's documentaion I think this function should return one string. It is probably better if you use rust-bitcoin for this. For the block case you can use MerkleBlock::from_header_txids_with_predicate and then serialize_hex. For the cached ones it's harder, I don't think there's an easy solution.
There was a problem hiding this comment.
Okay, this change a lot of things to really make this core-like. Turning to draft again.
There was a problem hiding this comment.
MerkleBlock doesnt have any serialize_hex, im trying to use consensus_encode() since its what might be the right way to do it... The only thing that might be wrong here is if rust-bitcoin enconding doesnt match cores. But im not being able to reproduce the command perfectly, the setup needed is quite boring to workout and floresta is giving me some strange bugs that i may post issues about.
There was a problem hiding this comment.
MerkleBlock doesnt have any serialize_hex
serialize_hex is a standalone function from bitcoin::consensus
There was a problem hiding this comment.
Ok, saw it... but it just does what im already doing... I`ll change the code for sanity but the result will be the same
bfd35e8 to
32699d5
Compare
127f778 to
248ceef
Compare
crates/floresta-cli/src/main.rs
Outdated
| #[command(name = "gettxoutproof")] | ||
| GetTxOutProof { | ||
| /// The transaction IDs to prove | ||
| #[arg( required = true, value_parser = parsers::jsonlike_array_of_txids)] | ||
| txids: std::vec::Vec<Txid>, // you need to specify the path of Vec https://github.com/clap-rs/clap/discussions/4695 | ||
|
|
||
| /// The block in which to look for the transactions | ||
| #[arg(short = 'b', long = "blockhash", last = true, required = false)] | ||
| blockhash: Option<BlockHash>, | ||
| }, |
There was a problem hiding this comment.
Im struggling with clap config aiming to match cores api but it seems impossible since the only way that i can pass blockhash is by either specifying a flag or passing it after a --
The way it is, you can call it like:
floresta-cli gettxoutproof '["77bc13bb64c290fc6a1be9f2b9141d84417329a416411ada2f035ee2f754bb86"]' -- 00000000000000000002266ea7f71eb350a9a55e5672136a51ce6534d42247ebin the end, youll need atleast to adaptate something to integrate floresta RPC where cores was
There was a problem hiding this comment.
This is because you made it into an option, rather than an argument. Remove all the annotation for the second field but #[arg(required = false)]
There was a problem hiding this comment.
nice, made it work!!
There was a problem hiding this comment.
floresta-cli gettxoutproof '["77bc13bb64c290fc6a1be9f2b9141d84417329a416411ada2f035ee2f754bb86" , "77bc13bb64c290fc6a1be9f2b9141d84417329a416411ada2f035ee2f754bb86"]' 00000000000000000002266ea7f71eb350a9a55e5672136a51ce6534d42247ebWhen i print with
println!("{:?}", txids);
println!("{:?}", blockhash);[77bc13bb64c290fc6a1be9f2b9141d84417329a416411ada2f035ee2f754bb86, 77bc13bb64c290fc6a1be9f2b9141d84417329a416411ada2f035ee2f754bb86]
Some(00000000000000000002266ea7f71eb350a9a55e5672136a51ce6534d42247eb)
248ceef to
19d5ff7
Compare
|
332e69d, made the command succeed exactly with cores api, you can check this by yourself. floresta-cli gettxoutproof '["5a4ebf66822b0b2d56bd9dc64ece0bc38ee7844a23ff1d7320a88c5fdb2ad3e2"]' 000000000043a8c0fd1d6f726790caa2a406010d19efd2780db27bdbbd93baf6
"01000000ba8b9cda965dd8e536670f9ddec10e53aab14b20bacad27b9137190000000000190760b278fe7b8565fda3b968b918d5fd997f993b23674c0af3b6fde300b38f33a5914ce6ed5b1b01e32f570200000002252bf9d75c4f481ebb6278d708257d1f12beb6dd30301d26c623f789b2ba6fc0e2d32adb5f8ca820731dff234a84e78ec30bce4ec69dbd562d0b2b8266bf4e5a0105"and this is from a bitcoin core node (Im using a gambiarra with jq and a public node) [nix-shell:~]$ btc-rpc gettxoutproof '["5a4ebf66822b0b2d56bd9dc64ece0bc38ee7844a23ff1d7320a88c5fdb2ad3e2"]' 000000000043a8c0fd1d6f726790caa2a406010d19efd2780db27bdbbd93baf6
{
"jsonrpc": "2.0",
"id": "curl",
"result": "01000000ba8b9cda965dd8e536670f9ddec10e53aab14b20bacad27b9137190000000000190760b278fe7b8565fda3b968b918d5fd997f993b23674c0af3b6fde300b38f33a5914ce6ed5b1b01e32f570200000002252bf9d75c4f481ebb6278d708257d1f12beb6dd30301d26c623f789b2ba6fc0e2d32adb5f8ca820731dff234a84e78ec30bce4ec69dbd562d0b2b8266bf4e5a0105"
} |
This commit present some changes for gettxoutproof, adding the optional blockhash input which will make the method interns differ, searching for the transactions inside the specified block, otherwise, the previous default behavior of getting it from the wallet cache remains. The method now receives an array of txids instead of just one. A unused #[allow(unused)] was also removed from merkle.rs Also, now named gettxoutproof, was gettxproof.
19d5ff7 to
332e69d
Compare
| @@ -21,6 +21,7 @@ serde = { version = "1.0", features = ["derive"] } | |||
| serde_json = "1.0" | |||
| anyhow = "1.0" | |||
| jsonrpc = { version = "0.18.0", optional = true, features = ["minreq_http"] } | |||
| thiserror = "2.0.12" | |||
There was a problem hiding this comment.
I think in #86 we've discussed we wouldn't use thiserror forward. Using the macros and manual derivation instead. So I think we shouldn't add it here.
| /// | ||
| /// A good example for these parsers is the ['parsers::jsonlike_array_of_txids'] | ||
| /// that permit to `gettxoutproof` to receive the txids input as '["txid1","txid2"]'. | ||
| pub mod parsers { |
There was a problem hiding this comment.
Do you intended to add more of these? Might be better to leave them in their own file.
| fn get_txout_proof(&self, txids: Vec<Txid>, blockhash: Option<BlockHash>) -> Result<String> { | ||
| let params: Vec<Value> = match blockhash { | ||
| Some(blockhash) => vec![ | ||
| serde_json::to_value(txids).unwrap(), |
There was a problem hiding this comment.
Remove the unwraps
| let tx = self.get_transaction(txid)?; | ||
| /// Returns the Merkle Proof for a given txid | ||
| /// | ||
| /// Fails if a given Txid is an unconfirmed transaction. |
There was a problem hiding this comment.
Returns None. Failing more like a function that returns Result
| @@ -28,6 +28,10 @@ impl MerkleProof { | |||
| pos: 0, | |||
| } | |||
| } | |||
| F438 | /// Return the hashes fot this proof as a [`Vec<String>`] | ||
| /// This function has two paths, when blockhash is inserted and when isn't. | ||
| /// | ||
| /// Specifying the blockhash will make this function go after the block and search | ||
| /// for the transcations inside it, building a merkle proof from the block with its |
There was a problem hiding this comment.
typo: transcations
| // Before building the merkle block we try to remove all txids | ||
| // that arent present in the block we found, meaning that | ||
| // at least one of the txids doesnt belong to the block which | ||
| // in case needs to make the command fails. | ||
| // | ||
| // this makes the use MerkleBlock::from_block_with_predicate useless. |
There was a problem hiding this comment.
| // Before building the merkle block we try to remove all txids | |
| // that arent present in the block we found, meaning that | |
| // at least one of the txids doesnt belong to the block which | |
| // in case needs to make the command fails. | |
| // | |
| // this makes the use MerkleBlock::from_block_with_predicate useless. | |
| // Before building the merkle block we try to remove all txids | |
| // that aren't present in the block we found, meaning that | |
| // at least one of the txids doesn't belong to the block which | |
| // in case needs to make the command fails. | |
| // | |
| // this makes the use MerkleBlock::from_block_with_predicate useless. |
| @@ -202,6 +203,7 @@ pub enum Error { | |||
| impl Display for Error { | |||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | |||
| match self { | |||
| Error::InvalidBlockHash => write!(f, "Inserted a invalid BlockHash"), | |||
There was a problem hiding this comment.
| Error::InvalidBlockHash => write!(f, "Inserted a invalid BlockHash"), | |
| Error::InvalidBlockHash => write!(f, "Provided a invalid BlockHash"), |
| .0 | ||
| .to_lower_hex_string(), | ||
| ) | ||
| .expect("GetTxOutProof implements serde")) | ||
| } |
There was a problem hiding this comment.
Nit empty line after the end of gettxoutproof
| let blockhash = if params.len() > 1 { | ||
| Some( | ||
| serde_json::from_value(params[1].clone()) | ||
| .map_err(|_| Error::InvalidBlockHash)?, | ||
| ) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
I think you can make this more clear with combinators
| let blockhash = if params.len() > 1 { | |
| Some( | |
| serde_json::from_value(params[1].clone()) | |
| .map_err(|_| Error::InvalidBlockHash)?, | |
| ) | |
| } else { | |
| None | |
| }; | |
| let blockhash = params | |
| .get(1) | |
| .cloned() | |
| .map(serde_json::from_value) | |
| .transpose() | |
| .map_err(|_| Error::InvalidBlockHash)?; |
What is the purpose of this pull request?
Which crates are being modified?
Description
Fix #378
This commit present some changes for gettxoutproof, adding the optional blockhash
input which will make the method interns differ, searching for the transactions
inside the specified block, otherwise, the previous default behavior of getting it
from the wallet cache remains. The method now receives an array of txids instead of
just one.
Also, now named gettxoutproof, was gettxproof.
A unused #[allow(unused)] was also removed from merkle.rs
Notes to the reviewers
While trying to fork and just complete #379 to accomplish one of
0.8.0 milestonesi noticed some opportunities to optimize some decisions and add two new fixes to the #378 issue. (1. and 2.)Still validating if the changes are effective and are working.
Author Checklist
just pcc(recommended but slower)just lint-features '-- -D warnings' && cargo test --releaseFinally, you are encouraged to sign all your commits (it proves authorship and guards against tampering—see How (and why) to sign Git commits and GitHub's guide to signing commits).