Merge ElementsProject/elements#1592: blind/blindpsbt fixes
What changed, and why it matters
This merge commit fixes several security and robustness bugs in Elements' confidential-transaction blinding code and in its dynamic-federated (dynafed) header handling. The most user-visible issues are in 'blindpsbt': malformed PSET inputs could previously crash the node (assertion failures), read memory out of bounds, or trick a verifier into accepting a wrong amount. Other changes stop a zero-input transaction from crashing during surjection-proof creation, make the range-proof cache depend on more data so it cannot return a wrong 'already verified' answer, and ensure dynafed block headers always carry and validate their height even when a legacy option is off. The pull request says the issues were 'picked up during LLM scans' and does not claim any are already exploited in the wild.
Treat this as a security-relevant maintenance merge and include it in the next release. Users and integrators who process untrusted PSETs or run Elements nodes on dynafed chains should upgrade. No emergency response is indicated by the supplied materials, but the denial-of-service and proof-verification fixes are worth backporting to supported release branches.
Security signals we found
Replacement of assertion failures with recoverable error returns in attacker-influenced PSET blinding paths
Out-of-bounds read prevention by requiring 33-byte Pedersen commitments before passing buffers to libsecp256k1
Range-proof equality verification now checks both min and max bounds, closing a proof-forgery window
Rejection of off-curve blinding public keys before ECDH, preventing a process-abort assert
Rejection of nullopt output amounts in PSET v0 blinding, preventing undefined dereference
Rejection of empty surjection-target set, preventing a downstream cryptographic assertion
Range-proof cache key expanded to include asset commitment and scriptPubKey, reducing collision risk
Dynafed header height always reconstructed and validated, fixing consensus-relevant header identity
Dynafed parameter transition threshold computed with overflow-safe ceiling
Evidence from the diff
The commit bundles nine fixes. In src/blind.cpp, SurjectOutput now rejects an empty surjection-target set, preventing a secp256k1_surjectionproof_initialize assertion on zero-input, multi-blindable-output transactions. In src/blindpsbt.cpp, CreateAssetSurjectionProof replaces two asserts with recoverable false returns; VerifyBlindValueProof now requires genuine 33-byte commitments for both value and asset (not merely non-null) and checks both rangeproof min and max bounds against the claimed amount; BlindPSBT refuses to blind a PSET v0 output whose amount is nullopt and validates the blinding pubkey with IsFullyValid() before ECDH. src/script/sigcache.{cpp,h} expands the range-proof cache key to include the asset commitment and scriptPubKey, closing a cache-collision path. src/chain.h, src/validation.cpp and src/dynafed.cpp correct dynafed header reconstruction/validation: dynafed headers always serialize block_height, so it must be reconstructed and validated regardless of g_con_blockheightinheader, and the 4/5 vote threshold is computed without integer-overflow rounding. src/headerssync.{cpp,h} retains Elements-specific header fields (block_height, proof, dynafed params, signblock witness) in CompressedHeader so presync/redownload can reconstruct signed/dynafed headers faithfully.
Changed components
src/blind.cppsrc/blindpsbt.cppsrc/script/sigcache.cppsrc/script/sigcache.hsrc/chain.hsrc/validation.cppsrc/dynafed.cppsrc/headerssync.cppsrc/headerssync.hInspect captured patch +86 / −18
### src/blind.cpp
@@ -206,9 +206,12 @@ bool SurjectOutput(CTxOutWitness& txoutwit, const std::vector<secp256k1_fixed_as
// with more than 256 inputs. The Elements verification code will always try to give
// secp-zkp the complete list of inputs, and if this exceeds 256 then surjectionproof_verify
// will always return false, so there is no way to work around this situation at signing time
- if (surjection_targets.size() > SECP256K1_SURJECTIONPROOF_MAX_N_INPUTS) {
+ if (surjection_targets.empty() || surjection_targets.size() > SECP256K1_SURJECTIONPROOF_MAX_N_INPUTS) {
// We must return false here to avoid triggering an assertion within
- // secp256k1_surjectionproof_initialize on the next line.
+ // secp256k1_surjectionproof_initialize on the next line: the
+ // cryptographic API requires a non-empty set of surjection targets,
+ // and the raw-blinding path can reach us with an empty vector
+ // (zero-input tx with multiple blindable outputs).
return false;
}
// Find correlation between asset tag and listed input tags
### src/blindpsbt.cpp
@@ -52,10 +52,17 @@ bool CreateAssetSurjectionProof(std::vector<unsigned char>& output_proof, const
}
// Using the input chosen, build proof
ret = secp256k1_surjectionproof_generate(secp256k1_blind_context, &proof, &ephemeral_input_tags[0], ephemeral_input_tags.size(), &output_asset_tag, input_index, input_asset_blinders[input_index].begin(), output_asset_blinder.begin());
- assert(ret == 1);
+ if (ret != 1) {
+ // Attacker-selected tags/generators without a known discrete-log
+ // relationship cause generation to fail; this must be a recoverable
+ // PSET error, not a process abort.
+ return false;
+ }
// Double-check answer
ret = secp256k1_surjectionproof_verify(secp256k1_blind_context, &proof, &ephemeral_input_tags[0], ephemeral_input_tags.size(), &output_asset_tag);
- assert(ret == 1);
+ if (ret != 1) {
+ return false;
+ }
// Serialize into output witness structure
size_t output_len = secp256k1_surjectionproof_serialized_size(secp256k1_blind_context, &proof);
@@ -189,7 +196,11 @@ bool CreateBlindAssetProof(std::vector<unsigned char>& assetproof, const CAsset&
bool VerifyBlindValueProof(CAmount value, const CConfidentialValue& conf_value, const std::vector<unsigned char>& proof, const CConfidentialAsset& conf_asset)
{
- if (conf_value.IsNull() || conf_asset.IsNull()) {
+ // The value and asset must be genuine commitments (33-byte, PrefixA/B)
+ // before their buffers are handed to libsecp256k1, which consumes exactly
+ // 33 serialized bytes. An explicit 9-byte value (or a null field) must not
+ // reach the parser, which would otherwise read out of bounds.
+ if (!conf_value.IsCommitment() || !conf_asset.IsCommitment()) {
return false;
}
@@ -208,7 +219,11 @@ bool VerifyBlindValueProof(CAmount value, const CConfidentialValue& conf_value,
if (secp256k1_rangeproof_verify(secp256k1_blind_context, &min_value, &max_value, &value_commit, proof.data(), proof.size(), /* extra_commit */ nullptr, /* extra_commit_len */ 0, &gen) == 0) {
return false;
}
- return min_value == (uint64_t)value;
+ // A range-membership proof is only meaningful as an equality proof if the
+ // proven interval collapses to the claimed amount. Comparing solely the
+ // lower bound would accept a proof whose committed value is larger than
+ // the displayed amount. Require both bounds to equal `value`.
+ return min_value == (uint64_t)value && max_value == (uint64_t)value;
}
BlindProofResult VerifyBlindProofs(const PSBTOutput& o) {
@@ -500,6 +515,14 @@ BlindingStatus BlindPSBT(PartiallySignedTransaction& psbt, std::map<uint32_t, st
// Check this is our output to blind
if (output.m_blinder_index == std::nullopt || our_input_data.count(*output.m_blinder_index) == 0) continue;
+ // PSET v0 does not require an output amount (it is only enforced for
+ // m_psbt_version >= 2), so a crafted v0 PSET can reach the blinding
+ // loop with output.amount == nullopt. Dereferencing it is undefined
+ // behaviour. Refuse to blind such an output.
+ if (output.amount == std::nullopt) {
+ return BlindingStatus::INVALID_BLINDER;
+ }
+
// Things we are going to stuff into the PSBTOutput if everything is successful
CConfidentialValue value_commitment;
CConfidentialAsset asset_commitment;
@@ -555,6 +578,13 @@ BlindingStatus BlindPSBT(PartiallySignedTransaction& psbt, std::map<uint32_t, st
CreateValueCommitment(value_commitment, value_commit, value_blinder, asset_generator, *output.amount);
// Generate rangproof nonce
+ if (!output.m_blinding_pubkey.IsFullyValid()) {
+ // An attacker-controlled (off-curve) blinding pubkey would otherwise
+ // reach CKey::ECDH, whose only validation is an assert on the peer
+ // key, aborting the process. The non-PSET path (blind.cpp) requires
+ // IsFullyValid() before ECDH; mirror it here.
+ return BlindingStatus::INVALID_BLINDER;
+ }
uint256 nonce = GenerateRangeproofECDHKey(ecdh_key, output.m_blinding_pubkey);
// Generate rangeproof
### src/chain.h
@@ -315,7 +315,10 @@ class CBlockIndex
block.hashPrevBlock = pprev->GetBlockHash();
block.hashMerkleRoot = hashMerkleRoot;
block.nTime = nTime;
- if (g_con_blockheightinheader) {
+ // Dynafed headers always serialize block_height as part of their
+ // identity (see CBlockHeader::Serialize), so it must be reconstructed
+ // regardless of the legacy -con_blockheightinheader option.
+ if (g_con_blockheightinheader || is_dynafed_block()) {
block.block_height = nHeight;
}
block.nBits = nBits;
@@ -540,7 +543,10 @@ class CDiskBlockIndex : public CBlockIndex
block.hashPrevBlock = hashPrev;
block.hashMerkleRoot = hashMerkleRoot;
block.nTime = nTime;
- if (g_con_blockheightinheader) {
+ // Dynafed headers always serialize block_height as part of their
+ // identity (see CBlockHeader::Serialize), so it must be reconstructed
+ // regardless of the legacy -con_blockheightinheader option.
+ if (g_con_blockheightinheader || is_dynafed_block()) {
block.block_height = nHeight;
}
block.nBits = nBits;
### src/dynafed.cpp
@@ -14,6 +14,10 @@ bool NextBlockIsParameterTransition(const CBlockIndex* pindexPrev, const Consens
}
std::map<uint256, uint32_t> vote_tally;
assert(next_height >= consensus.dynamic_epoch_length);
+ // Require at least four-fifths of the epoch's votes. (epoch_length*4)/5
+ // floor-divides, under-approximating the 80% threshold for epoch lengths
+ // not divisible by 5; N - N/5 is the overflow-safe ceiling of N*4/5.
+ const uint32_t threshold = consensus.dynamic_epoch_length - consensus.dynamic_epoch_length / 5;
for (int32_t height = next_height - 1; height >= (int32_t)(next_height - consensus.dynamic_epoch_length); --height) {
const CBlockIndex* p_epoch_walk = pindexPrev->GetAncestor(height);
assert(p_epoch_walk);
@@ -25,8 +29,7 @@ bool NextBlockIsParameterTransition(const CBlockIndex* pindexPrev, const Consens
const uint256 proposal_root = proposal.CalculateRoot();
vote_tally[proposal_root]++;
// Short-circuit once 4/5 threshold is reached
- if (!proposal_root.IsNull() && vote_tally[proposal_root] >=
- (consensus.dynamic_epoch_length*4)/5) {
+ if (!proposal_root.IsNull() && vote_tally[proposal_root] >= threshold) {
winning_entry = proposal;
return true;
}
### src/headerssync.cpp
@@ -19,9 +19,15 @@ constexpr size_t HEADER_COMMITMENT_PERIOD{624};
//! received and validated against commitments.
constexpr size_t REDOWNLOAD_BUFFER_SIZE{14827}; // 14827/624 = ~23.8 commitments
-// Our memory analysis assumes 48 bytes for a CompressedHeader (so we should
-// re-calculate parameters if we compress further)
-static_assert(sizeof(CompressedHeader) == 48);
+// NOTE (ELEMENTS): The upstream Bitcoin memory analysis assumed 48 bytes for
+// a CompressedHeader, which holds only the PoW fields. Elements must retain
+// the identity/proof fields (block_height, proof, dynafed params, signblock
+// witness) so that signed/dynafed headers can be reconstructed faithfully, so
+// CompressedHeader is now larger than 48 bytes. The redownload buffer is
+// bounded by REDOWNLOAD_BUFFER_SIZE headers per peer, so the per-peer memory
+// cost is REDOWNLOAD_BUFFER_SIZE * sizeof(CompressedHeader); this remains
+// small but should be reconsidered if REDOWNLOAD_BUFFER_SIZE is ever raised.
+static_assert(sizeof(CompressedHeader) <= 512);
HeadersSyncState::HeadersSyncState(NodeId id, const Consensus::Params& consensus_params,
const CBlockIndex* chain_start, const arith_uint256& minimum_required_work) :
### src/headerssync.h
@@ -25,6 +25,15 @@ struct CompressedHeader {
uint32_t nTime{0};
uint32_t nBits{0};
uint32_t nNonce{0};
+ // ELEMENTS: fields needed to faithfully reconstruct signed/dynafed
+ // headers. These participate in the header's identity (block_height,
+ // dynafed params, proof challenge) or are required for downstream
+ // validation (proof solution, signblock witness), so they must be retained
+ // across the headers-sync presync/redownload path.
+ uint32_t block_height{0};
+ CProof proof;
+ DynaFedParams m_dynafed_params;
+ CScriptWitness m_signblock_witness;
CompressedHeader()
{
@@ -38,6 +47,10 @@ struct CompressedHeader {
nTime = header.nTime;
nBits = header.nBits;
nNonce = header.nNonce;
+ block_height = header.block_height;
+ proof = header.proof;
+ m_dynafed_params = header.m_dynafed_params;
+ m_signblock_witness = header.m_signblock_witness;
}
CBlockHeader GetFullHeader(const uint256& hash_prev_block) {
@@ -46,8 +59,12 @@ struct CompressedHeader {
ret.hashPrevBlock = hash_prev_block;
ret.hashMerkleRoot = hashMerkleRoot;
ret.nTime = nTime;
+ ret.block_height = block_height;
ret.nBits = nBits;
ret.nNonce = nNonce;
+ ret.proof = proof;
+ ret.m_dynafed_params = m_dynafed_params;
+ ret.m_signblock_witness = m_signblock_witness;
return ret;
};
};
### src/script/sigcache.cpp
@@ -55,9 +55,9 @@ void SignatureCache::ComputeEntrySchnorr(uint256& entry, const uint256& hash, Sp
}
// ELEMENTS:
-void SignatureCache::ComputeEntryRangeProof(uint256& entry, const std::vector<unsigned char>& proof, const std::vector<unsigned char>& commitment) const {
+void SignatureCache::ComputeEntryRangeProof(uint256& entry, const std::vector<unsigned char>& proof, const std::vector<unsigned char>& commitment, const std::vector<unsigned char>& asset_commitment, const CScript& scriptPubKey) const {
CSHA256 hasher = m_salted_hasher_range_proof;
- hasher.Write(proof.data(), proof.size()).Write(commitment.data(), commitment.size()).Finalize(entry.begin());
+ hasher.Write(proof.data(), proof.size()).Write(commitment.data(), commitment.size()).Write(asset_commitment.data(), asset_commitment.size()).Write(scriptPubKey.data(), scriptPubKey.size()).Finalize(entry.begin());
}
void SignatureCache::ComputeEntrySurjectionProof(uint256& entry, const uint256 &hash, const std::vector<unsigned char>& proof, const std::vector<unsigned char>& commitment) const {
CSHA256 hasher = m_salted_hasher_surjection_proof;
@@ -131,7 +131,7 @@ bool InitSurjectionproofCache(size_t max_size_bytes)
bool CachingRangeProofChecker::VerifyRangeProof(const std::vector<unsigned char>& vchRangeProof, const std::vector<unsigned char>& vchValueCommitment, const std::vector<unsigned char>& vchAssetCommitment, const CScript& scriptPubKey, const secp256k1_context* secp256k1_ctx_verify_amounts) const
{
uint256 entry;
- rangeProofCache.ComputeEntryRangeProof(entry, vchRangeProof, vchValueCommitment);
+ rangeProofCache.ComputeEntryRangeProof(entry, vchRangeProof, vchValueCommitment, vchAssetCommitment, scriptPubKey);
if (rangeProofCache.Get(entry, !store)) {
return true;
### src/script/sigcache.h
@@ -84,7 +84,7 @@ class SignatureCache
void ComputeEntrySchnorr(uint256& entry, const uint256 &hash, Span<const unsigned char> sig, const XOnlyPubKey& pubkey) const;
// ELEMENTS:
- void ComputeEntryRangeProof(uint256& entry, const std::vector<unsigned char>& proof, const std::vector<unsigned char>& commitment) const;
+ void ComputeEntryRangeProof(uint256& entry, const std::vector<unsigned char>& proof, const std::vector<unsigned char>& commitment, const std::vector<unsigned char>& asset_commitment, const CScript& scriptPubKey) const;
void ComputeEntrySurjectionProof(uint256& entry, const uint256 &hash, const std::vector<unsigned char>& proof, const std::vector<unsigned char>& commitment) const;
### src/validation.cpp
@@ -4845,7 +4845,10 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, BlockValidatio
return state.Invalid(BlockValidationResult::BLOCK_INVALID_HEADER, "time-too-old", "block's timestamp is too early");
// Check height in header against prev
- if (g_con_blockheightinheader && (uint32_t)nHeight != block.block_height) {
+ // Dynafed headers always serialize block_height as part of their identity
+ // (see CBlockHeader::Serialize), so the height must be validated even when
+ // the legacy -con_blockheightinheader option is disabled.
+ if ((g_con_blockheightinheader || !block.m_dynafed_params.IsNull()) && (uint32_t)nHeight != block.block_height) {
LogPrintf("ERROR: %s: block height in header is incorrect (got %d, expected %d)\n", __func__, block.block_height, nHeight);
return state.Invalid(BlockValidationResult::BLOCK_INVALID_HEADER, "bad-header-height");
}Why this scored 72/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.