[RPCSAGA] verifyutxochaintipinclusionproof rpc#1085
Conversation
b03c945 to
854b248
Compare
|
Needs rebase |
|
NACK on renaming. This RPC will be used by integrations that wishes to have utreexo support, we shouldn't make |
854b248 to
0116df4
Compare
makes sense if the idea is to stick with the reference name but, the point is that Ive been thinking on a alias feature for the rpc, with it i could fix this naming and build other interesting shortcuts without raising inconsistency in the common interface |
a0f981f to
1ea4355
Compare
|
Rebased. |
|
|
||
| let is_valid = stump | ||
| .verify(&proof.proof, &proof.hashes_proven) | ||
| .map_err(|e| JsonRpcError::InvalidProof(format!("Proof verification failed: {e:?}")))?; |
There was a problem hiding this comment.
Proof error implement Display, no?
There was a problem hiding this comment.
StumpError doesn’t implement std::fmt::Display
| } | ||
| } | ||
|
|
||
| impl Decodable for TipProof { |
There was a problem hiding this comment.
Fuzz for this and the FromStr above.
| // Hashes proven (u32 LE count + 32-byte hashes) | ||
| let num_proven = u32::consensus_decode(reader)? as usize; | ||
| if num_proven > MAX_INPUTS_PER_BLOCK { | ||
| return Err(bitcoin::consensus::encode::Error::ParseFailed( | ||
| "Too many proven hashes", | ||
| )); | ||
| } |
There was a problem hiding this comment.
Why didn't you use read_bounded_len?
There was a problem hiding this comment.
The value im trying to extract is a u32, AFAIK read_bounded_len will only work for VarInt
b6479f0 to
1454d31
Compare
|
Applied @Davidson-Souza suggestions.
|
| blockhash: Option<BlockHash>, | ||
| ) -> Result<VerifyUtxoChainTipInclusionProofRes, JsonRpcError> { | ||
| // The hash we got querying for the given blockhash | ||
| let internal_hash = blockhash.unwrap_or(self.get_best_block_hash()?); |
There was a problem hiding this comment.
rust will compute the argument in unwrap_or before it knows whether blockhash is Some or None...this is bad bcus if get_best_block_hash fails.... the user sees that error even though they provided a hash that would have worked
let internal_hash = match blockhash {
Some(h) => h,
None => self.get_best_block_hash()?,
};
or u could use unwrap_or_else
There was a problem hiding this comment.
get_best_block_hash() should be logically infallible, but the suggestion makes sense.
Ill add about get_best_block_hash() case to our error handling overhaul.
820a8e8 to
ec080e5
Compare
| fuzz_target!(|data: &[u8]| { | ||
| let _ = deserialize::<TipProof>(data); | ||
| }); |
ec080e5 to
4461dc9
Compare
|
Done @Davidson-Souza |
Rename `acc()` to `get_acc(block: Option<BlockHash>)` in the `BlockchainInterface` trait, enabling callers to retrieve the accumulator state for a specific historical block. When `None` is passed, the current tip accumulator is returned (preserving the previous behavior). - Implement historical lookup in `ChainState` via `get_disk_block_header` + `get_roots_for_block` - Remove the duplicate `get_acc()` from `UpdatableChainstate` - Update all `acc()` call sites to use BlockchainInterface::get_acc().
Implement verification of Utreexo accumulator inclusion proofs for chain tip UTXOs. Accepts hex-encoded proofs from utreexod's proveutxochaintipinclusion RPC and validates them against the internal Utreexo accumulator. Includes: - TipProof type with consensus decoding and hex parsing - Verbosity 0: returns boolean validation result - Verbosity 1: returns detailed proof information - Stale proof detection (chain tip mismatch) - Optional blockhash parameter for historical verification - DoS protection (4MB max proof size) - Comprehensive parsing unit tests - RPC documentation
Add the `verifyutxochaintipinclusionproof` subcommand and its RPC client method to `FlorestaRPC`. Accepts a hex proof string, optional verbosity level (0 or 1), and optional blockhash parameter.
Comprehensive functional test suite for the verifyutxochaintipinclusionproof RPC: - Valid proof with verbosity 0 and 1 - Explicit blockhash verification - Stale proof with blockhash fallback - Invalid hex, malformed proof, oversized proof - Invalid verbosity level
…uzz targets Add two fuzz targets for the TipProof type: - tip_proof_des: raw bytes deserialization via consensus_decode - tip_proof_from_str: hex string parsing via FromStr - tip_proof_roundtrip: roundtrip symmetry
4461dc9 to
0d995fc
Compare
|
Rebased |
| // This is the block we will reorg to | ||
| fork_acc = chain.acc(); | ||
| fork_acc = chain | ||
| .get_acc(None) |
There was a problem hiding this comment.
Perhaps a get_acc and get_tip_acc would be better to avoid all those None there.
|
|
||
| fn acc(&self) -> Stump { | ||
| read_lock!(self).acc.to_owned() | ||
| fn get_acc(&self, block: Option<BlockHash>) -> Result<Stump, Self::Error> { |
There was a problem hiding this comment.
Same thing about Result<Option<T>> I've mentioned a couple times.
| @@ -941,9 +941,6 @@ impl<PersistedState: ChainStore> ChainState<PersistedState> { | |||
| None => Ok(true), | |||
There was a problem hiding this comment.
Would be nice to have tests for the arbitrary block acc.
There was a problem hiding this comment.
|
|
||
| /// Verbosity one, with more detailed information about the proof | ||
| One(VerifyUtxoChainTipInclusionProofVerbose), | ||
| } |
There was a problem hiding this comment.
We could have a verbosity level wrapper like:
#[derive(Debug, Serialize, Deserialize)]
#[serde(untagged)]
pub enum VerbosityLevel<Z: Debug, Serialize, Deserialize , O: Debug, Serialize, Deserialize> {
Zero(Z),
One(O),
}There was a problem hiding this comment.
Perhaps this suggestion belongs to #1076 and should be applied together with the input type ?
The current implementation follows the same pattern as the other methods that have verbosity, and we could work this suggestion for them all in a single pr.
| /// | ||
| /// <https://bitcoin.stackexchange.com/questions/85752/maximum-number-of-inputs-per-transaction> | ||
| const MAX_INPUTS_PER_BLOCK: usize = 24_386; | ||
| pub const MAX_INPUTS_PER_BLOCK: usize = 24_386; |
There was a problem hiding this comment.
Don't think this needs to be public
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| /// A chain-tip inclusion proof as serialized by utreexod. | ||
| /// | ||
| /// Responses to `proveutxochaintipinclusion` utreexod RPC. | ||
| pub struct TipProof { | ||
| /// The block hash at which this proof was generated. | ||
| pub proved_at_hash: BlockHash, | ||
|
|
||
| /// The Utreexo accumulator proof (targets + sibling hashes). | ||
| pub proof: Proof, | ||
|
|
||
| /// The raw leaf hashes that were proven. | ||
| pub hashes_proven: Vec<BitcoinNodeHash>, | ||
| } |
There was a problem hiding this comment.
Think I already mentioned this, but this seems offtopic for wire. This struct should be in rpc
| } | ||
|
|
||
| fuzz_target!(|data: &[u8]| { | ||
| if let Ok(inp) = Inputs::arbitrary(&mut Unstructured::new(data)) { |
There was a problem hiding this comment.
This isn't really what I've suggested. Look at the *rtt targets we already have.
This proof was generated by utreexod. Edit: It looks like the problem are coinbase transactions, for some reason. |
|
Can you share the command that you used to generate the proof ? |
|
Description and Notes
Superseeds #819.
Implement Utreexod's
verifyutxochaintipinclusionproofTo implement the feature of specifying a blockhash to control the block which we should verify the proof against i had to make some changes on our blockchain backend only to expose the option.
We had three main external call points to fetch the acc from the blockchain, always the tip, BlockchainInterface::acc(), ChainState::acc() and UpdatableChainstate::get_acc(), now they are centralized into BlockChainInterface::get_acc().
This PR also includes extensive tests, unit tests for the new request structure parsing and integration tests for the rpc itself.