multi: make v3 transaction standard (no TRUC impl yet) #2432
Conversation
Pull Request Test Coverage Report for Build 20323834190Details
💛 - Coveralls |
|
cc: @bhandras for review |
integration/v3_transaction_test.go
Outdated
| testVersionedTransaction(t, r, 1, true) | ||
| }) | ||
|
|
||
| // MAke sure that v2 transactions are still accepted. |
integration/v3_transaction_test.go
Outdated
|
|
||
| // TestV3TransactionChaining tests that v3 transactions can be chained | ||
| // (spending outputs from other v3 transactions). | ||
| func TestV3TransactionChaining(t *testing.T) { |
There was a problem hiding this comment.
The test isn't doing what it's claiming to do.
There's no code guaranteeing that the inputs for tx2 will be the outputs from tx1.
And doing a simple println shows that the transactions are not being chained.
[I] u0_a306@localhost ~/b/integration (2025-12-04-tmp-test)> go test -tags=rpctest -run=TestV3TransactionChaining
2025-12-04 13:10:52.187 [INF] ITEST: Established connection to RPC server 127.0.0.1:8335
2025-12-04 13:10:52.502 [INF] ITEST: Established connection to RPC server 127.0.0.1:8335
2025-12-04 13:10:52.670 [INF] ITEST: Established connection to RPC server 127.0.0.1:8337
2025-12-04 13:10:52.981 [INF] ITEST: Established connection to RPC server 127.0.0.1:8337
2025-12-04 13:10:53.192 [DBG] ITEST: Detected btcd version: 250000
tx1: 04dd06c83b204a6bc3c6034df8a11306eb7c460e1ac535090bac98c355d2ae01
04dd06c83b204a6bc3c6034df8a11306eb7c460e1ac535090bac98c355d2ae01 0
04dd06c83b204a6bc3c6034df8a11306eb7c460e1ac535090bac98c355d2ae01 1
tx2 in a6ad4800142e85e8a7ffe289eb8f97acefcd5b9cda1d5d1e799a15cbc7d2a5f3:0
tx2: 337115644e3a8195a7ff1ef240c1205a2cf11ce3250a60d8024f5c9d46bbd392
Added code:
293 ▎ │ fmt.Println("tx1:", tx1.TxHash().String())
1 ▎ │ for i := range tx1.TxOut {
2 ▎ │ │ fmt.Println(tx1.TxHash().String(), i)
3 ▎ │ }
4 ▎ │ for _, txIn := range tx2.TxIn {
5 ▎ │ │ fmt.Println("tx2 in", txIn.PreviousOutPoint.String())
6 ▎ │ }
7 ▎ │ fmt.Println("tx2:", tx2.TxHash().String())
There was a problem hiding this comment.
I decided to just remove this test, it doesn't really assert anything relevant. It was part of a scratch pad before I started to implement the other TRUC logic.
|
Most of it looks good. |
yyforyongyu
left a comment
yyforyongyu
left a comment
There was a problem hiding this comment.
Looking close! I wonder if we could do a side branch instead, then we can just merge this PR, and address the comments in a new PR, which makes it easier to keep the momentum.
| MaxSigOpCostPerTx: blockchain.MaxBlockSigOpsCost / 4, | ||
| MinRelayTxFee: cfg.minRelayTxFee, | ||
| MaxTxVersion: 2, | ||
| MaxTxVersion: mempool.MaxStandardTxVersion, |
There was a problem hiding this comment.
a minor point: given that TRUC is not implemented, should we move this to a side branch instead?
There was a problem hiding this comment.
Even though we haven't implemented it, right now we won't relay any of the transactions even though they're valid. Also us not supporting it at the package level here prevents devs from easily creating and interacting with these transactions.
Relay is a bit more flexible, than say consensus in this case.
There was a problem hiding this comment.
Without this, if a dev was using btcd for first hop relay, and wanted to use P2A or TRUC or w/e (relying on the rest of the network to implement the policy rules), btcd would reject it as non-standard.
integration/v3_transaction_test.go
Outdated
| mempool, err = r.Client.GetRawMempool() | ||
| require.NoError(t, err) | ||
| require.NotContains(t, mempool, tx2Hash) | ||
| } |
There was a problem hiding this comment.
cool - tho I think we should also add the case that when tx1 is unconfirmed, we assert that tx2 can spend it, given that TRUC is about enforcing new policy in the mempool.
There was a problem hiding this comment.
I decided to just remove this test, it doesn't really assert anything relevant. It was part of a scratch pad before I started to implement the other TRUC logic.
This commit introduces the MaxStandardTxVersion constant to the mempool policy package, setting it to 3 to enable support for version 3 transactions. Previously, the maximum accepted transaction version was hardcoded to 2 in the server configuration. Note that we don't yet implement the new TRUC rules, yet. But this'll enable acceptance into the btcd mempool.
In this commit, we replace the hardcoded transaction version limit of 2 with the MaxStandardTxVersion constant from the mempool package. This change ensures the server configuration stays synchronized with the mempool policy definition and enables v3 transaction support throughout the system.
In this commit, we introduce a functional options pattern to the test memwallet, allowing tests to specify custom transaction versions when creating transactions. This enhancement provides the test infrastructure needed to validate v3 transaction handling throughout the system. All transaction creation methods (CreateTransaction, SendOutputs, and SendOutputsWithoutChange) now accept variadic options while maintaining backward compatibility - existing tests continue to work unchanged as the default behavior remains version 1 transactions.
In this commit, we extend the mempool policy test suite to explicitly verify that version 2 and version 3 transactions are accepted as standard. The test now uses the MaxStandardTxVersion constant rather than a hardcoded value, ensuring the tests stay synchronized with the actual policy implementation. We added specific test cases for v2 and v3 transactions to document and verify the expected behavior, confirming that transactions with these versions pass the standardness checks. The test for excessive version numbers was also updated to use MaxStandardTxVersion + 1, making it clear that we're testing the rejection of versions beyond our current policy limit.
In this commit, we introduce a comprehensive integration test suite that validates v3 transaction handling across the full stack, from mempool acceptance through mining into blocks. These tests ensure that v3 transactions work correctly in real-world scenarios using the RPC test harness. The test suite covers multiple critical scenarios to ensure robust v3 transaction support. TestV3TransactionPolicy validates that transactions with versions 1, 2, and 3 are accepted while version 4 is properly rejected, confirming our policy enforcement works as intended. The test also verifies that accepted v3 transactions can be successfully mined into blocks, demonstrating end-to-end functionality. TestV3TransactionRPCSubmission specifically tests the RPC pathway, ensuring that v3 transactions created with the WithTxVersion option can be submitted through SendOutputs and properly confirmed in blocks. This validates that the functional options we added to the test infrastructure work correctly in practice. TestV3TransactionChaining goes further by testing that outputs from v3 transactions can be spent by subsequent transactions, confirming that the UTXO management and transaction validation logic properly handles the new version throughout the transaction lifecycle.
Roasbeef
force-pushed
the
v3-tx-standard
branch
from
591f482 to
4b7dbe4
Compare
|
Ok seems like things are good. Just want to sanity check on this though #2432 (comment) |
6 participants
In this commit, we make a series of changes to make v3 transactions standard. This means that we'll relay, then and accept them into our mempool. This PR doesn't yet implement the new TRUC semantics yet.