Explore possible refactors or additional test cases in btcd/wire/msgtx.go

[otech47]

Given recent changes to this file in #1896 and #1907 were needed recently to patch and ship hotfixes in LND (ex: lightningnetwork/lnd#7096 & lightningnetwork/lnd#7002) it may be worth looking into whether some of these functions rely on, or are affected by, bitcoin consensus code so they can be moved/removed/refactored to reduce the chances of consensus conflicts and provide stronger assumptions for other software layers that may rely on btcd for consensus.

Some analysis of the file:

• 1 dependency on chaincfg/chainhash
• 20 constant values are defined, 2 of which involve maximum values for the number of witness items and their total size
• 7 type/struct definitions used throughout
• 36 functions
• 1 variable

The file is also covered by 10 test suites in msgtx_test.go (with multiple tests each) but the file does not seem to have been updated for/after taproot activation and perhaps could use further test cases to account for edge cases or non-standard transactions

Does this seem like a worthwhile approach?

If the block weight limit were to be doubled as a consensus change tomorrow, would this file need to be changed to enforce consensus?

If so, can (should?) those changes be isolated and extracted into an explicitly marked consensus layer so that other software that depends on /btcd/wire can use the types/getters/other utility functions independently of consensus assumptions?

My proficiency in go is not very high so I wanted to source feedback before diving deeper into these questions

[guggero]

Refactor for the sake of refactor is often discouraged in critical code such as this. As refactor changes are hard to review and always bring the potential of introducing new bugs. So it's likely a big refactor PR won't be picked up for review very quickly.

But improving test coverage and adding specific test cases around some of the updated limits with Taproot consensus code would definitely be welcome IMO.

[Roasbeef]

bitcoin consensus code so they can be moved/removed/refactored to reduce the chances of consensus conflicts and provide stronger assumptions for other software layers that may rely on btcd for consensus.

Certainly, we had some clean up planned before the latest incident. In terms of optimizations in this area, there're a few PRs that lack review that make some notable strides:

• wire/common: Implement binarySerializer based on sync.Pool #1686
• wire: only borrow/return binaryFreeList buffers at the message level #1426

One low hanging fruit is modifying the script test vectors to use the same code to parse the witness/transaction data as the p2p/wire code. This would have caught the first issue (witness size), but not the second (number of witness elements, check bypassed thru OP_SUCESS. Oliver has a good point above, but on the upside this is wire code and the consensus code hasn't needed to be touched in a while.

If the block weight limit were to be doubled as a consensus change tomorrow, would this file need to be changed to enforce consensus?

Yes, if block weight doubled over night, in a way that wasn't actually backwards compatible (so increasing the value instead of an extension block), we'd need to adjust things. IMO, the way the witness discount was implemented was sort of a one time thing, since all peers have that as a hard limit now, and increasing it further likely requires extensions blocks or a hardfork.

FWIW, bitcoind would also need to be updated as well. In the past they had a limit of 2 MB. This didn't cause an issue w/ segwit, since old peers never actually see the witness data (just 1 MB blocks). This was then later raised to 4 MB, which is the value just about everyone uses now. There's another value that governs the max payload read, which is ~32 MB.

If so, can (should?) those changes be isolated and extracted into an explicitly marked consensus layer so that other software that depends on /btcd/wire can use the types/getters/other utility functions independently of consensus assumptions?

IMO things as is are already pretty cleanly separated. All the consensus level stuff lives in the blockchain and txscript packages. The original mistake I made here 6 years ago was adding these additional DoS constants outside of just the max 4 MB payload limiting.

[Roasbeef]

updated for/after taproot activation and perhaps could use further test cases to account for edge cases or non-standard transactions

I think for this we'd want to refresh the current fuzz testing harness, and also extract out some of the policy stuff further to make writing stuff like differential fuzz tests easier.

[otech47]

the current fuzz testing harness

are you referring to msgtx_test.go? or is there a different set of files for this?

I assume you mean these structures

and that refreshing them would involve considering other transaction constructions that are now possible with taproot locking/unlocking scripts?

[otech47]

I've got a basic start I'll try chipping away at in #1917 thanks for all the feedback

One low hanging fruit is modifying the script test vectors to use the same code to parse the witness/transaction data as the p2p/wire code.

By this do you mean write tests that call functions like ReadTxOut, readScript, etc instead of using BtcDecode which calls those inside of it? or am i missing something?

will also look at those other 2 PRs to understand past efforts at optimizing this part of the code