Prevent 64 byte txs at consensus level

[ajtowns]

Implement the no-64-byte-tx rule (CVE-2017-12842) from the Great Consensus Cleanup.

Forward ported from bitcoin#15482 (does not include anti-timewarp changes, or the script/sighash changes). Only rejects txs that serialize without the witness to exactly 64 bytes.

[instagibbs]

Looks like a faithful port; including the lack of test for the case :P

[maflcko]

lack of test for the case :P

https://drahtbot.space/host_reports/DrahtBot/reports/coverage/bitcoin/bitcoin/4b012eea18ad3031/total.coverage/index.html

[naumenkogs]

Looks good.

[ajtowns]

Comment fixed, test coverage added, and rebased

[ajtowns]

Needs a spec written up.

[ajtowns]

Rebased to 25.x

[ajtowns]

Rebased to 27.x

[ajtowns]

Rebased to 28.x

[darosior]

utACK

[instagibbs]

throwing this out there in case it makes sense based on the docs

Suggested change

	{
	{
	Assume(!block.fChecked);

is it important to run this check specifically before the merkle checks?

[ajtowns]

If you want to remove the call to CheckBlock prior to AcceptBlock in ProcessNewBlock then it's important to issue a BLOCK_MUTATED error for blocks with all 64-byte txs at a similar time to the merkle checks, as otherwise you'll get a real error which AcceptBlock will cache, preventing you from seeing the real block.

If you don't want to remove that call, then it matters a lot less where the 64b tx check is made.

As far as block.fChecked goes, seems to me it's more the reverse: you'd want ContextualBlockPreCheck() to set block.m_prechecked = true; and CheckBlock() to verify that's already been set?

[Christewart]

IIUC there are two test cases that are missing

1. Serializing a transaction with a witness that results in a 64 byte tx on the wire (but less than 64 bytes without the witness) should result in a valid block
2. In the SizeSub64 test, if we add witness data we should still have an invalid block (i.e. confirm witness data cannot circumvent the check).

I'm working on a draft BIP rn, if no one gets to these before I finish that i can add. Likely in ~10 days.

[Christewart]

IIUC there are two test cases that are missing

1. Serializing a transaction _with a witness_ that results in a 64 byte tx on the wire (but less than 64 bytes without the witness) should result in a valid block

2. In the `SizeSub64` test, if we add witness data we should still have an invalid block (i.e. confirm witness data cannot circumvent the check).

I'm working on a draft BIP rn, if no one gets to these before I finish that i can add. Likely in ~10 days.

Done here: ajtowns#13

[Christewart]

Does anyone have thoughts on adding the 64-byte transaction check to CheckTransaction()? I've done that here: ajtowns@b47f910

This allows for static test vectors to be written for this consensus proposal (added in tx_invalid.json). This does bleed through to the p2p layer causing us to now disconnect peers more eagerly if they propagate a 64 byte transaction: ajtowns@3a57e3d

EDIT: Thinking about this more, it seems like this starts enforcing the consensus change before its signaled on the network so the change isn't helpful. Ig static test vectors will have to wait?

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #99 (BIP54 "Consensus Cleanup" implementation by darosior)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[ajtowns]

Closing in favour of #99