unify coinbase_tx_prefix and coinbase_tx_suffix serialization
#1491
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1491 +/- ##
==========================================
+ Coverage 17.89% 17.94% +0.05%
==========================================
Files 132 132
Lines 10067 10118 +51
==========================================
+ Hits 1801 1816 +15
- Misses 8266 8302 +36
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
plebhash
force-pushed
the
refactor-stripped-coinbase
branch
3 times, most recently
from
aea059a to
c41c445
Compare
plebhash
force-pushed
the
refactor-stripped-coinbase
branch
10 times, most recently
from
13ef7c3 to
57e4cf4
Compare
plebhash
force-pushed
the
refactor-stripped-coinbase
branch
6 times, most recently
from
b1cef1e to
4f927dc
Compare
|
Is this ready for review @plebhash ? |
yes |
plebhash
force-pushed
the
refactor-stripped-coinbase
branch
from
4f927dc to
bc2da3d
Compare
|
@plebhash Did you consider using https://rust-unofficial.github.io/patterns/patterns/creational/builder.html for the new |
| #[derive(Clone)] | ||
| pub struct Coinbase { | ||
| pub tx: Transaction, | ||
| pub script_sig_prefix_len: usize, |
There was a problem hiding this comment.
Why do we need to save this?
There was a problem hiding this comment.
coinbase_tx_prefix consists of the serialized bytes of:
- tx version
- input count
- input OutPoint
- input index
- script_sig length
- coinbase_script_sig_prefix
2-6 are obtained by simply serializing the input, but we cannot take all those bytes because while extranonce is also part of script_sig, it's not part of coinbase_tx_prefix
so we to truncate this serialization, and Coinbase.script_sig_prefix_len is used to know where to truncate
plebhash
force-pushed
the
refactor-stripped-coinbase
branch
2 times, most recently
from
3100b32 to
3ab87d0
Compare
I didn't consider this while working on the PR. After your suggestion, I made some quick sketching but TBH I fail to see the advantages over the current approach. The commit is on a separate branch plebhash@e6278fb |
the functions for serializing `coinbase_tx_prefix` and `coinbase_tx_suffix` did not take into consideration that they must strip the coinbase from BIP141 data (marker + flag + witness) before calculating the merkle root this was patched via PR stratum-mining#488 although it fixed the perceived issues, it also introduced API fragmentation, since there's two different pairs of methods for serializing these fields... this is very confusing for the user we have: - `fn coinbase_tx_prefix(...)` vs `StrippedCoinbaseTx::into_coinbase_tx_prefix(...)` - `fn coinbase_tx_suffix(...)` vs `StrippedCoinbaseTx::into_coinbase_tx_suffix(...)` one includes BIP141 data, while the other removes it... and only the prefix + suffix without BIP141 are actually used for merkle root calculation this commit introduces a new `struct Coinbase`, which unifies coinbase abstractions, effectively replacing: - `struct StrippedCoinbase` (plus methods) - `fn coinbase -> Result<Transaction, Error>` (plus accompanying prefix and suffix functions) in practice, this means that now we **always** remove BIP141 data when serializing `coinbase_tx_prefix` and `coinbase_tx_suffix`... and most importantly: there's only one function responsible for this task, leaving no room for confusion by API users.
because of the previous commit, this function is no longer necessary
otherwise we will submit invalid blocks
plebhash
force-pushed
the
refactor-stripped-coinbase
branch
from
3ab87d0 to
f844005
Compare
There was a problem hiding this comment.
TBH I find the Coinbase API a bit confusing to me. There are instances where it is initialized with Coinbase { a, b }, others with Coinbase::new and also ::reconstruct.
I would prefer to have a new with docs above it indicating is it using segwit or not, and other one ::new_with/out_segwit. It also seems that if it is not segwit the user is expected to build it by themselves which I don't think is optimal.
| /// The function takes the coinbase_tx_prefix, extranonce, and coinbase_tx_suffix | ||
| /// and constructs a new coinbase transaction with the BIP141 marker and flag bytes | ||
| /// inserted after the version bytes. | ||
| pub fn reconstruct_with_bip141( |
There was a problem hiding this comment.
I think new_with_bip141 or new_with_segwit is more readable
| @@ -271,6 +271,67 @@ impl Coinbase { | |||
| coinbase_tx_suffix.extend_from_slice(&lock_time_u32.to_le_bytes()); | |||
| coinbase_tx_suffix.try_into().map_err(Error::BinarySv2Error) | |||
| } | |||
|
|
|||
| /// Reconstructs the coinbase transaction with BIP141 marker and flag bytes. | |||
| /// This is needed when constructing a block with SegWit transactions. | |||
There was a problem hiding this comment.
Is this needed because a block contains segwit transactions or because the coinbase is segwit, i.e., even if the block does have segwit txs but coinbase is not, this should not be used right?
There was a problem hiding this comment.
I expected this new function would spark some important discussions that even touch some spec-related topics.
I added this new function yesterday because I did some e2e testing mining on our custom signet and blocks were being rejected with:
2025-03-05T01:56:02Z [error] AcceptBlock: bad-witness-nonce-size, CheckWitnessMalleation : invalid witness reserved value size
2025-03-05T01:56:02Z [error] ProcessNewBlock: AcceptBlock FAILED (bad-witness-nonce-size, CheckWitnessMalleation : invalid witness reser
ved value size)
These logs didn't happen on the node that received the SubmitSolution, but rather on a peer.
(the fact that we didn't catch this on our CI is a bit concerning, but it's probably a separate discussion)
After some research I found this blogpost which described a similar scenario that happened in production last year: https://b10c.me/observations/10-viabtc-blocks-without-witness-data/
Coming back to some basic consensus rules:
In order for a block header to be valid, it's merkle_root field must always be calculated based on the Coinbase's txid, not wtxid.
That does not necessarily mean the Coinbase must be stripped of BIP141 data.
Miners could still theoretically produce consensus-valid Coinbases without BIP141 data. This only means that we need to strip BIP141 data from it in order to calculate it's txid.
However, miners are encouraged to always produce Coinbases with BIP141, as it would be economically irrational to do otherwise.
On SRI we assume miners will always produce Coinbases with BIP141.
The discussion here is centered around how the Coinbase is serialized for extended jobs to be sent downstream (since mining devices only care about finding a header with a valid merkle_root, thus only care about txid).
Coming back to some historical context for this PR:
coinbase_tx_prefix and coinbase_tx_suffix were originally implemented in roles_logic_sv2::job_creator in a way that they included BIP141 (segwit marker + flag + witness) fields during serialization
however, Sv1 firmware expects coinbase_tx_prefix and coinbase_tx_suffix to not include it (as Sv1 was conceived pre-segwit), because hashing coinbase_tx_prefix + extranonce + coinbase_tx_suffix leads to wtxid
so later on it was noted that this was breaking Sv1 compatibility, and then #488 introduced a new struct StrippedCoinbase to address this
PR #488 aimed to fix this issue with a very narrow scope, where BIP141 fields were stripped only during translation to Sv1
the Sv2 spec doesn't really say anything about this, and AFAIU it remains an open question, as I noticed that this comment from @Fi3 #488 (comment) was never properly addressed on PR #488
So now we come to a discussion about spec.
The open question is: what assumptions does Sv2 make for coinbase_tx_prefix + coinbase_tx_suffix?
or more specifically: on NewExtendedMiningJob and DeclareMiningJob, do we assume they are stripped from BIP141 data?
- if yes: then every Sv2-compliant software must always re-insert the BIP141 data while reconstructing a block for propagation (which is the direction this PR is going)
- if no: then functionality for stripping BIP141 data must be restricted to tProxy (which is what the code does before this PR, but with very confusing APIs)
I started this PR going in direction 1, but now I'm having second thoughts.
There was a problem hiding this comment.
Maybe as a start lets add the validation functionality from bitcoin core regarding blocks_without_witness_data to SRI and then rethink this
There was a problem hiding this comment.
no strong opinion, but I completely agree that we should address this and make the spec more clear about how bip141 bytes are handled. From what I understand you are proposing 2 options.
- "inside" sv2 we never have the witness bytes, that means that we don't have to worry about old miners getting translated jobs with the witness. But it also mean that the TP must strip witness bytes from the prefix and re-add them plus and the reserved value when we submit solution. We can easily enforce this property inside the SRI codebase (no witness around) and I don't think that will make the spec much more complex (we just need to add a small paragraph)
- strip the bytes before translating. This is fine as well. There is not a simple property that we can enforce like no witness data around. Is easier to build a TP that don't have to strip and add bytes. I think that is more future proof but didn't thought about it.
So I guess that we should decide if we want the burden of handling coinbase witness on sv2 or on a TP (that use an sv2 library that expect to get stripped data). I think that the latter is less error prone, since the sv2 library will enforce stripped data and will fail fast on most related TP issue. But not strong opinion.
I don't know if having an sv2 protocol witness agnostic is future proof, but maybe is more safe. Also there is only one TP and it is still in development but there is a lot of old sv1 software that do not know about the witness.
There was a problem hiding this comment.
Maybe as a start lets add the validation functionality from bitcoin core regarding blocks_without_witness_data to SRI and then rethink this
with the current implementation this error will never happen because SRI always creates a coinbase with full BIP141 data
this error was introduced in my PR while re-constructing the coinbase from serialized bytes, where BIP141 data were stripped
I fixed it with the commit that adds reconstruct_with_bip141 but that kickstarted the discussion about whether this is a future-proof approach
There was a problem hiding this comment.
But it also mean that the TP must strip witness bytes from the prefix and re-add them plus and the reserved value when we submit solution.
I don't think this would imply any changes on TP. The NewTemplate message doesn't contain any information about BIP141.
If you check the coinbase function you will see that SRI builds the coinbase from the separate fields that TP sent via NewTemplate. This function also uses APIs from rust-bitcoin to build a Coinbase transaction with BIP141 support.
In other words, SRI is currently making an opinionated assumption that all Sv2 Coinbases will always contain BIP141 support, and simply removing BIP141 data during Sv1 translation.
And after reflection and discussions, I think this is the safer approach, as always stripping BIP141 data from the coinbase could have unintended consequences in the future.
| @@ -1331,4 +1408,59 @@ mod tests { | |||
| // m.super_safe_lock(|i| *i = (*i).checked_add(1).unwrap()); // will not compile | |||
| m.super_safe_lock(|i| *i = (*i).checked_add(1).unwrap_or_default()); // compiles | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_reconstruct_with_bip141() { | |||
There was a problem hiding this comment.
It feels like the new code is testing itself here because you are comparing if the results from Coinbase::new and Coinbase::reconstruct_from.. are similar
| // and subtracting the extranonce length (32 in this case) | ||
| let extranonce_len = 32; | ||
| let script_sig_len = tx.input[0].script_sig.len(); | ||
| let script_sig_prefix_len = script_sig_len - extranonce_len; |
There was a problem hiding this comment.
would it make sense to save the right side (extranonce_len here) instead of script_sig_prefix_len? it feels like we are doing this calculation but are not using it until we call other function. maybe the responsibility of doing this should be on the other function.
|
reverting this back to draft as I'm unsure about the current approach |
close #1490
to be merged after #1442 and #1461
we introduce a new
struct Coinbaseintoroles_logic_sv2::utilsthat replaces the following fromroles_logic_sv2::job_creator:struct StrippedCoinbasefn coinbase -> Result<Transaction, Error>