re-introduce dedicated function to deserialize outputs from NewTemplate message#1777
Merged
plebhash merged 1 commit intoJul 8, 2025
Conversation
7ff316c to
bedb09a
Compare
as the outcome of Sjors/bitcoin#92
bedb09a to
9633259
Compare
Shourya742
reviewed
Jul 8, 2025
Comment on lines
+70
to
+99
| let mut deserialized_outputs: Vec<TxOut> = vec![]; | ||
|
|
||
| // The serialized outputs are in Bitcoin consensus format | ||
| // We need to parse them one by one, keeping track of cursor position | ||
| let mut cursor = 0; | ||
| let mut txouts = &serialized_outputs[cursor..]; | ||
|
|
||
| // Iteratively decode each TxOut until we can't decode any more | ||
| while let Ok(out) = TxOut::consensus_decode(&mut txouts) { | ||
| // Calculate the size of this TxOut based on its script_pubkey length | ||
| // 8 bytes for value + variable bytes for script_pubkey length | ||
| // For small scripts (0-252 bytes): 1 byte length prefix | ||
| // For medium scripts (253-1000000 bytes): 3 byte length prefix (1 marker + 2 byte | ||
| // length) | ||
| let len = match out.script_pubkey.len() { | ||
| a @ 0..=252 => 8 + 1 + a, // 8 (value) + 1 (compact size) + script_len | ||
| a @ 253..=1000000 => 8 + 3 + a, // 8 (value) + 3 (compact size) + script_len | ||
| _ => break, // Unreasonably large script, likely an error | ||
| }; | ||
|
|
||
| // Move the cursor forward by the size of this TxOut | ||
| cursor += len; | ||
| deserialized_outputs.push(out); | ||
| } | ||
|
|
||
| if deserialized_outputs.len() != coinbase_tx_outputs_count as usize { | ||
| return Err(Error::FailedToDeserializeCoinbaseOutputs); | ||
| } | ||
|
|
||
| Ok(deserialized_outputs) |
Member
There was a problem hiding this comment.
Just curious, can we use a Cursor directly here, since consensus_decode already accepts any type that implements Read? This way, we wouldn't need to manually update the buffer position, as consensus_decode would advance the cursor on its own.
Something like this:
pub fn deserialize_template_outputs_1(
serialized_outputs: Vec<u8>,
coinbase_tx_outputs_count: u32,
) -> Result<Vec<TxOut>, Error> {
let mut cursor = Cursor::new(serialized_outputs);
let mut deserialized_outputs = Vec::new();
while let Ok(out) = TxOut::consensus_decode(&mut cursor) {
deserialized_outputs.push(out);
}
Ok(deserialized_outputs)
}
Member
Author
There was a problem hiding this comment.
sure, feel free to send a PR with this improvement
but please make sure this is not left out, because that's the reason coinbase_tx_outputs_count is used as an input parameter for this function:
if deserialized_outputs.len() != coinbase_tx_outputs_count as usize {
return Err(Error::FailedToDeserializeCoinbaseOutputs);
}
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
3 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.
as the outcome of Sjors/bitcoin#92
to be merged after #1775
in #1772 we realized that
deserialize_outputswas introducing issues into the codethat's because this function can only deserialize outputs that were serialized into an opaque blob, not following Bitcoin consensus rules (setting a
VarIntprefix as the size of the array, followed by the serialized bytes... or in other words,rust-bitcoinserializing aVec<TxOut>)this function was being used to deserialize outputs from
SetCustomMiningJob, which are serialized under Bitcoin consensus rules (Vec<TxOut>)NewTemplate.coinbase_tx_outputshowever follows a different rule, whereNewTemplate.coinbase_tx_outputs_countcarries the size of the array, andNewTemplate.coinbase_tx_outputssimply has the raw bytes (noVarIntprefix to indicate the size of the array)in other words,
NewTemplate.coinbase_tx_outputsMUST NOT be deserialized viaVec::<TxOut>::consensus_decode, and it needs a dedicated function to do sobut
SetCustomMiningJob.coinbase_tx_outpusMUST be deserialized viaVec::<TxOut>::consensus_decode(and this is what was causing problems on #1771)