multi: make anchor tx version configurable and validate v3 with bitcoind#1857
multi: make anchor tx version configurable and validate v3 with bitcoind#1857bhandras wants to merge 7 commits intolightninglabs:mainfrom
Conversation
Summary of ChangesHello @bhandras, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request systematically updates the default Bitcoin transaction version to 3 (TRUC) across various components of the system. This foundational change is implemented to leverage the improved security features of BIP 431, such as enhanced fee-bumping and pinning protection, which are critical for the robustness of Taproot Assets. The update is accompanied by comprehensive testing to confirm the new version's adoption and ensure seamless integration with existing data. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly updates the Bitcoin transaction version from 2 to 3 across the codebase, aligning with BIP 431 for TRUC transactions. The change is consistently applied, and the inclusion of new tests to verify the transaction version and ensure backward compatibility with v2 proofs is a great addition. My review includes a couple of suggestions to improve code clarity and maintainability.
proof/verifier_test.go
Outdated
| proof := proofs[0] | ||
|
|
||
| require.Equal(t, int32(3), proof.AnchorTx.Version, | ||
| "Post-commit b3562461: all new transactions should be v3") |
There was a problem hiding this comment.
It's great that you've included a commit hash in this assertion message. However, it might be even more helpful to also include a reference to the BIP (BIP 431) that introduces v3 transactions. This would provide more context for future developers.
tapsend/send.go
Outdated
| } | ||
|
|
||
| txTemplate := wire.NewMsgTx(2) | ||
| // TODO(bhandras) |
There was a problem hiding this comment.
This TODO was added without any context. Could you please clarify what this refers to or remove it if it's not needed?
tapsend/send.go
Outdated
|
|
||
| txTemplate := wire.NewMsgTx(2) | ||
| // TODO(bhandras) | ||
| txTemplate := wire.NewMsgTx(3) |
There was a problem hiding this comment.
To improve maintainability and avoid the use of magic numbers, it would be beneficial to define a constant for the new transaction version, for example const BitcoinTxVersion3 = 3. This constant could then be used here and in all other places in this PR where the transaction version is set to 3.
| txTemplate := wire.NewMsgTx(3) | |
| txTemplate := wire.NewMsgTx(BitcoinTxVersion3) |
Pull Request Test Coverage Report for Build 19974072375Details
💛 - Coveralls |
Roasbeef
left a comment
Roasbeef
left a comment
There was a problem hiding this comment.
It'll be a larger change, but rather than may all transactions 3 by default, I think we should expose this on the internal API and also the gRPC level. Until a few months ago, v3 transactions were actually non standard. We want to ensure that our transactions propagate as widely as possible. v3 transactions also opt into new rules which may not always apply in our context.
One good way to this would be to use a functional option for relevant APIs.
|
@bhandras, remember to re-request review from reviewers when ready |
|
Picking this up again now that lntest supports bitcoind miner. |
bhandras
force-pushed
the
v3-temp
branch
2 times, most recently
from
b64b41c to
c0df0e5
Compare
bhandras
changed the title
github-project-automation
bot
moved this from 🆕 New
to 👀 In review
in Taproot-Assets Project Board
bhandras
force-pushed
the
v3-temp
branch
7 times, most recently
from
f38ffdd to
b7e498e
Compare
Update the integration harnesses to the current lnd miner APIs and the dependency snapshot they require. - Bump lnd to the latest master snapshot and refresh the dependent module graph. - Replace direct btcd RPC miner usage in the itest harnesses with lntest miner helpers. - Adjust the broad integration test suite and helper code to the new miner interfaces. - Fix the mock gRPC TLS helpers to advertise h2 so courier tests keep passing with the newer gRPC stack. - Keep these changes focused on the harness migration and compatibility fallout, leaving the dedicated v3 coverage for a follow-up commit.
Separate the unrelated integration test stabilizations from the harness migration so the miner API update stays easier to review. - Wait for canceled custom-channel hodl payments to leave the in-flight state before probing the route with a follow-up payment. - Wait for supply-commit updates to actually reach the mempool before mining the follow-on commitment transaction. - Relax the proof-transfer transfer-list assertions to rely on the confirmation height hint instead of the lagging block hash field. - Keep the coverage focused on existing flaky paths that showed up in CI, without changing the anchor-version behavior under test.
- Wait for the shared MultiSubscription to report active subscriptions again after the mock server restarts before publishing msg2. - The test already waited for the two direct clients, but not for the multi-subscription, which left a race where the post-restart message could be published before that consumer had reconnected. - Because this subscription uses an empty filter, a missed publish is not recovered from backlog delivery and the test times out on readMultiSub. - Keep the fix test-only so mailbox runtime behavior is unchanged. - This makes TestServerClientAuthAndRestart pass reliably under race runs and matches the failure seen in CI shard 0.
Add the RPC surface for choosing the version of newly created anchor transactions. - Add the shared AnchorTxVersion enum to the common proto definitions. - Thread the enum into the mint, send, and anchor-virtual-PSBT requests. - Regenerate the protobuf and swagger artifacts for the updated schemas. - Parse the requested version in the RPC server and map unspecified to the daemon default. - Leave CommitVirtualPsbts unchanged because callers already control the anchor PSBT they submit.
Thread the selected anchor transaction version through the code paths that actually construct new Bitcoin anchor transactions. - Add a small tapsend option layer with a default of v2 and explicit v3 support. - Pass the selected version through send, mint, and supply-commit flows instead of changing every transaction template in the codebase. - Keep virtual transactions pinned to v2 because existing witness signatures depend on that version. - Carry the selected version through the supply-commit state machine while keeping persisted mint events backward compatible. - Set the daemon-side default for new supply-commit transactions to the standard anchor transaction default.
Cover the restored default-v2 behavior and the explicit-v3 paths with focused unit and subsystem tests. - Assert that the default proof and anchor construction paths still create v2 anchor transactions. - Add direct tapsend coverage for selecting the anchor transaction version. - Update supply-commit test fixtures to use v3 where the new explicit version is exercised. - Extend the supply-commit manager mock expectations to include the propagated anchor transaction version. - Keep the existing proof compatibility check in place for older v2 proof files.
Add a dedicated bitcoind-backed integration lane for the explicit v3 anchor transaction paths. - Add an end-to-end itest that mints, sends, and anchors PSBT transfers with v3 anchor transactions. - Add the helper assertions and mint options needed to request and verify v3 anchors in the test harness. - Register the new case in the itest list and add an itest-v3 make target. - Install bitcoind in CI and add a dedicated workflow job for the v3 lane. - Keep the existing broader integration suite unchanged while giving the v3 path its own focused coverage.
4 participants
Summary
This PR changes anchor transaction version handling from a blanket default to explicit configuration on the flows that actually create anchor transactions.
The daemon still defaults to v2. Callers can now explicitly request v3 for newly created anchor transactions in the minting, send, supply-commit, and PSBT anchoring paths.
What Changed
tapsend,tapgarden,tapfreighter, anduniverse/supplycommit, while keeping v2 as the default.lndto the latest master and update the itest harnesses and helpers to the currentlntestminer APIs.bitcoindin CI, and add a dedicated bitcoind-backeditest-v3target and workflow job.h2so the courier-related tests keep passing with the newer gRPC stack pulled in by thelndbump.Why
Testing
go test ./itest/...make itest-v3go test ./proof ./tapchannelgo test ./...