Merge bitcoin/bitcoin#36168: PSBT: Make input/output `Merge()` methods return void
What the developer wrote
Authored by merge-script
Merge bitcoin/bitcoin#36168: PSBT: Make input/output `Merge()` methods return void
82deb69111f15db60aa364db9b06931918691564 PSBT: Make input/output `Merge()` methods return void (nebula-21)
Pull request description:
PSBT input/output `Merge()` methods always return `true` unconditionally and have no failure paths. As a result, the return value checks in `PartiallySignedTransaction::Merge()` can never fail and are dead code.
This makes the `bool` return type and `[[nodiscard]]` misleading.
This PR changes both methods to return `void` and remove the return value checks. If at some point in the future the failure logic is needed, it can be introduced again. For now using `void` makes the current behavior easier to understand.
ACKs for top commit:
achow101:
ACK 82deb69111f15db60aa364db9b06931918691564
polespinasa:
ACK 82deb69111f15db60aa364db9b06931918691564
sedited:
ACK 82deb69111f15db60aa364db9b06931918691564
Tree-SHA512: 694fdf19292d2f3627c90c5f111d7ecf5ad4f933d69eebf7c9ef37adee19be7de44002030ed4539aa5a28910acb751822b443f7ee22c80614296ef40d21c60da
What changed, and why it matters
This commit is a straightforward code cleanup in Bitcoin Core's Partially Signed Bitcoin Transaction (PSBT) handling. It changes two internal methods from returning true (always, with no way to fail) to returning nothing at all, and removes now-unnecessary error checks. There is no security bug being fixed and no change in behavior for users.
No security action required. Treat as normal code-quality refactor.
Security signals we found
No strong security signals were identified.
Evidence from the diff
The commit refactors PSBTInput::Merge() and PSBTOutput::Merge() from bool to void, removes their unconditional return true; statements, drops the [[nodiscard]] attribute, and removes the if (!...Merge(...)) return false; checks in PartiallySignedTransaction::Merge(). The PR description explicitly states these methods had no failure paths, so the bool return and nodiscard were misleading dead code. This is a pure maintainability/correctness cleanup with no functional or security impact.
Changed components
src/psbt.cppsrc/psbt.hPSBTInput::Merge()PSBTOutput::Merge()PartiallySignedTransaction::Merge()Inspect captured patch +6 / −14
### src/psbt.cpp
@@ -47,14 +47,10 @@ bool PartiallySignedTransaction::Merge(const PartiallySignedTransaction& psbt)
}
for (unsigned int i = 0; i < inputs.size(); ++i) {
- if (!inputs[i].Merge(psbt.inputs[i])) {
- return false;
- }
+ inputs[i].Merge(psbt.inputs[i]);
}
for (unsigned int i = 0; i < outputs.size(); ++i) {
- if (!outputs[i].Merge(psbt.outputs[i])) {
- return false;
- }
+ outputs[i].Merge(psbt.outputs[i]);
}
MergeGlobalXPubs(psbt);
if (fallback_locktime == std::nullopt && psbt.fallback_locktime != std::nullopt) fallback_locktime = psbt.fallback_locktime;
@@ -421,7 +417,7 @@ void PSBTInput::FromSignatureData(const SignatureData& sigdata)
}
}
-bool PSBTInput::Merge(const PSBTInput& input)
+void PSBTInput::Merge(const PSBTInput& input)
{
if (!non_witness_utxo && input.non_witness_utxo) non_witness_utxo = input.non_witness_utxo;
if (witness_utxo.IsNull() && !input.witness_utxo.IsNull()) {
@@ -467,8 +463,6 @@ bool PSBTInput::Merge(const PSBTInput& input)
if (sequence == std::nullopt && input.sequence != std::nullopt) sequence = input.sequence;
if (time_locktime == std::nullopt && input.time_locktime != std::nullopt) time_locktime = input.time_locktime;
if (height_locktime == std::nullopt && input.height_locktime != std::nullopt) height_locktime = input.height_locktime;
-
- return true;
}
bool PSBTInput::HasSignatures() const
@@ -535,7 +529,7 @@ void PSBTOutput::FromSignatureData(const SignatureData& sigdata)
m_musig2_participants.insert(sigdata.musig2_pubkeys.begin(), sigdata.musig2_pubkeys.end());
}
-bool PSBTOutput::Merge(const PSBTOutput& output)
+void PSBTOutput::Merge(const PSBTOutput& output)
{
hd_keypaths.insert(output.hd_keypaths.begin(), output.hd_keypaths.end());
m_proprietary.insert(output.m_proprietary.begin(), output.m_proprietary.end());
@@ -547,8 +541,6 @@ bool PSBTOutput::Merge(const PSBTOutput& output)
if (m_tap_internal_key.IsNull() && !output.m_tap_internal_key.IsNull()) m_tap_internal_key = output.m_tap_internal_key;
if (m_tap_tree.empty() && !output.m_tap_tree.empty()) m_tap_tree = output.m_tap_tree;
m_musig2_participants.insert(output.m_musig2_participants.begin(), output.m_musig2_participants.end());
-
- return true;
}
bool PSBTInputSigned(const PSBTInput& input)
### src/psbt.h
@@ -324,7 +324,7 @@ class PSBTInput
void FillSignatureData(SignatureData& sigdata) const;
void FromSignatureData(const SignatureData& sigdata);
- [[nodiscard]] bool Merge(const PSBTInput& input);
+ void Merge(const PSBTInput& input);
uint32_t GetVersion() const { return m_psbt_version; }
COutPoint GetOutPoint() const;
/**
@@ -958,7 +958,7 @@ class PSBTOutput
void FillSignatureData(SignatureData& sigdata) const;
void FromSignatureData(const SignatureData& sigdata);
- [[nodiscard]] bool Merge(const PSBTOutput& output);
+ void Merge(const PSBTOutput& output);
uint32_t GetVersion() const { return m_psbt_version; }
explicit PSBTOutput(uint32_t psbt_version, CAmount amount, const CScript& script)Why this scored 15/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.