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

[DrahtBot]

There hasn't been much activity lately. What is the status here?

Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

[darosior]

Why is this complexity of performing contextual checks before CheckBlock necessary? I don't see a reason why we would need to not persist blocks invalid because they contain 64 bytes transactions as opposed to any other reason for invalidity.

[darosior]

Seems like this is just to be able to call it from inside ConnectBlock, and since it's there it might as well be called also along with CheckBlock.

[ajtowns]

Here's a sketch of two alternative approaches:

• ajtowns@0d565eb
  • adds contextual information to CheckBlock() (ie a chainman and pindexPrev pointer)
  • when all txs are 64 bytes, downgrade all errors to BLOCK_MUTATED
  • otherwise, if a 64 byte tx is found, and the 64 byte rule is enabled, gives a consensus failure
  • this would allow dropping the extra call to CheckBlock() in ProcessNewBlock()
• ajtowns@b601f93
  • moves the check into ContextualCheckBlock
  • (requires keeping the extra call to CheckBlock() in ProcessNewBlock() in order to downgrade those failures to as-if they were BLOCK_MUTATED)

[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?