blind/blindpsbt fixes - #1592
Merged
Merged
Conversation
The raw-blind RPC path can reach SurjectOutput with an empty surjection_targets vector (a zero-input tx with multiple blindable outputs), which indexed element [0] of the empty vector and passed it to secp256k1_surjectionproof_initialize, triggering undefined behaviour. Reject empty target sets up front, matching the existing over-limit guard.
An explicit 9-byte value (or a null field) passed the IsNull() check and its buffer was then parsed as a 33-byte Pedersen commitment, reading past the end. Require IsCommitment() on both the value and asset fields so the parser's length precondition holds and the out-of-bounds read is avoided.
…ueProof A range-membership proof whose lower bound equalled the displayed PSET amount was accepted even when the committed value was larger, because only min_value was compared. Require both verified bounds to equal the claimed amount so a proof can no longer understate an output's value.
A dynafed header always serializes block_height as part of its identity (CBlockHeader::Serialize/GetHash), independent of the legacy -con_blockheightinheader option. Previously the height was only validated in ContextualCheckBlockHeader and only reconstructed in CBlockIndex/CDiskBlockIndex::GetBlockHeader when that option was on, so a dynafed header with a mismatched height could be accepted, and a header rebuilt from an accepted index no longer matched the accepted header's hash when the option was off. Validate and reconstruct block_height for all dynafed headers regardless of the option. Non-dynafed headers keep the legacy option behaviour. This does not change any consensus rule: the height was already part of every dynafed header's hash.
NextBlockIsParameterTransition computed the approval threshold as (epoch_length*4)/5, which floor-divides. For epoch lengths not divisible by 5 this is below the intended at-least-four-fifths rule, so a transition could pass with fewer than 80% of the epoch's blocks voting for it. Use the overflow-safe ceiling N - N/5 (== ceil(N*4/5)). This is a no-op for epoch lengths divisible by 5 (the only currently deployed case) and only corrects the under-approximation for non-divisible epoch lengths.
CompressedHeader kept only the Bitcoin PoW fields and GetFullHeader() reconstructed only those. In the low-work headers presync/redownload path Elements headers were therefore rebuilt without block_height, proof, dynafed params, or the signblock witness, so the reconstructed header's hash no longer matched what was received and the fields signed/dynafed validation needs were dropped. Retain block_height, proof, m_dynafed_params, and m_signblock_witness in CompressedHeader and reconstruct them in GetFullHeader(). This enlarges the per-header redownload buffer footprint (previously assumed to be 48 bytes); the comment and static_assert are updated accordingly. No consensus rule changes: the reconstructed header now matches the header that was received.
BlindPSBT dereferenced output.amount without a nullopt check. A crafted v0 PSET output (m_blinder_index set, amount absent) reached the blinding loop and dereferenced a disengaged std::optional, which is undefined behaviour. Refuse such outputs with BlindingStatus::INVALID_BLINDER.
BlindPSBT passed the blinding pubkey straight to CKey::ECDH, whose only validation is an assert on the peer key, so a crafted off-curve pubkey (reaching IsBlinded() but failing IsFullyValid()) aborted the process. Mirror the non-PSET path and return BlindingStatus::INVALID_BLINDER when the pubkey is not fully valid.
CreateAssetSurjectionProof asserted on secp256k1_surjectionproof_generate and _verify failure. A crafted PSET can supply unrelated tags/generators with no known discrete-log relationship, causing generation to fail and the assert to abort the process. Make these recoverable errors by returning false.
Member
|
ACK c26d719 tested locally. (test each commit failing on master, unrelated to this PR) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a number of small issues picked up during LLM scans