Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update standardness rules congruent to Bitcoin Core #2178

Merged
merged 2 commits into from
May 21, 2024

Conversation

ProofOfKeags
Copy link
Contributor

This PR updates a couple of relay policy rules to bring them in line with what Bitcoin Core does. We do two things here,

  1. Make CODESEPARATOR non-standard in non-segwit scripts
  2. Make transactions <65 bytes (excluding the witness) non-standard

References:

@coveralls
Copy link

coveralls commented Apr 29, 2024

Pull Request Test Coverage Report for Build 9180393402

Details

  • 89 of 98 (90.82%) changed or added relevant lines in 6 files are covered.
  • 4 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.04%) to 56.907%

Changes Missing Coverage Covered Lines Changed/Added Lines %
txscript/sigvalidate.go 24 25 96.0%
mempool/mempool.go 1 4 25.0%
wire/msgtx.go 7 12 58.33%
Files with Coverage Reduction New Missed Lines %
connmgr/connmanager.go 1 86.27%
wire/msgtx.go 3 93.98%
Totals Coverage Status
Change from base Build 8881487645: 0.04%
Covered Lines: 29493
Relevant Lines: 51827

💛 - Coveralls

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice PR, there're a few lingering policy rules bitcoind applies that we don't, so nice to be able to periodically bridge the gap. Hopefully one day this'll all be in a nice BIP to make it easier to track relevant policy that full node implementations should consider.

Left a few comments in line

@@ -589,6 +596,12 @@ func (msg *MsgTx) btcDecode(r io.Reader, pver uint32, enc MessageEncoding,
sbuf = sbuf[len(txin.Witness[j]):]
}
}

// Check that if the witness flag is set that we actually have
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this is something that is actually spelled out in the old BIP (144):

If the witness is empty, the old serialization format must be used.

https://github.com/bitcoin/bips/blob/master/bip-0144.mediawiki

wire/msgtx_test.go Outdated Show resolved Hide resolved

// ScriptVerifyConstScriptCode fails non-segwit scripts if a signature
// match is found in the script code or if OP_CODESEPARATOR is used.
ScriptVerifyConstScriptCode
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for aligning here, this wasn't in the original set of BIPs, but was added afterwards: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2017-November/015292.html

Being closer to bitcoind here policy wise may make it easier to keep up with w/e the outcome of the Great Consensus Cleanup Revival is.

txscript/sigvalidate.go Outdated Show resolved Hide resolved
txscript/sigvalidate.go Outdated Show resolved Hide resolved
txscript/sigvalidate.go Show resolved Hide resolved
txscript/opcode.go Outdated Show resolved Hide resolved
This is to mitigate CVE-2017-12842. Along the way, also error when
deserializing transactions that have the witness marker flag set
but have no witnesses. This matches Bitcoin Core's behaviour initially
introduced here bitcoin/bitcoin#14039. Allowing
such transactions is benign, but this makes sure that our parsing code
matches Core's exactly.
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🧆

@Roasbeef Roasbeef merged commit c4ed92f into btcsuite:master May 21, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants