Do not store witness txn in rejection cache #8525

Merged
merged 2 commits into from Sep 9, 2016

Projects

None yet

5 participants

@sipa
Member
sipa commented Aug 16, 2016 edited

As proposed in the IRC meeting from august 4th (see http://www.erisian.com.au/meetbot/bitcoin-core-dev/2016/bitcoin-core-dev.2016-08-04-19.00.log.html), this changes the logic to never store witness transactions (or transactions whose witness data was stripped) in the rejection cache.

As an optimization, only double check for witness invalidity if the transaction has no witness. This changes the meaning for state.CorruptionPossible() for transactions from "witness possibly invalidated" to "witness stripped". This is faster (no need for 3 script checks) and simpler for the DoS logic.

This also removes the special logic to not assign DoS for witness failures for non-witness peers. That logic already exists in more generic form, through the non-mandatory script verify flags.

See also #8279.

@MarcoFalke MarcoFalke added this to the 0.13.1 milestone Aug 16, 2016
@instagibbs instagibbs and 1 other commented on an outdated diff Aug 16, 2016
@@ -5488,7 +5488,10 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
// Probably non-standard or insufficient fee/priority
LogPrint("mempool", " removed orphan tx %s\n", orphanHash.ToString());
vEraseQueue.push_back(orphanHash);
- if (!stateDummy.CorruptionPossible()) {
+ if (orphanTx.wit.IsNull() && !state.CorruptionPossible()) {
@instagibbs
instagibbs Aug 16, 2016 Contributor

s/state/stateDummy/ ?

@sipa
sipa Aug 16, 2016 Member

Nice catch, fixed!

@instagibbs
Contributor

code review ACK 59d0df0

@laanwj
Member
laanwj commented Aug 17, 2016

concept ACK 59d0df0

@sipa
Member
sipa commented Aug 18, 2016

@sdaftuar @morcos Comments?

@instagibbs
Contributor

created a tiny test for the witness-stuffing case that previous blinded nodes: instagibbs@62778b2

@sipa
Member
sipa commented Sep 5, 2016

Rebased, and incorporated @instagibbs' test.

@laanwj
Member
laanwj commented Sep 9, 2016

utACK ca10a03

@laanwj laanwj merged commit ca10a03 into bitcoin:master Sep 9, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@laanwj laanwj added a commit that referenced this pull request Sep 9, 2016
@laanwj laanwj Merge #8525: Do not store witness txn in rejection cache
ca10a03 Add basic test for IsStandard witness transaction blinding (instagibbs)
34521e4 Do not store witness txn in rejection cache (Pieter Wuille)
80a4f21
@sdaftuar
Contributor

@sipa Sorry for the late review! One question about this:

As an optimization, only double check for witness invalidity if the transaction has no witness. This changes the meaning for state.CorruptionPossible() for transactions from "witness possibly invalidated" to "witness stripped". This is faster (no need for 3 script checks) and simpler for the DoS logic.

We still have one place in AcceptToMemoryPoolWorker where we return a state with CorruptionPossible set to true, even when there is a witness:

https://github.com/sipa/bitcoin/blob/ca10a03addf70421893791c2c499e82fc494d60b/src/main.cpp#L1147

This doesn't seem to be a problem at all, because the caller is checking to see if the witness is NULL before checking CorruptionPossible, but I was wondering if the intention was to eliminate this type of usage?

@laanwj laanwj added a commit to laanwj/bitcoin that referenced this pull request Sep 26, 2016
@sipa @laanwj sipa + laanwj Do not store witness txn in rejection cache
Github-Pull: #8525
Rebased-From: 34521e4
1672225
@laanwj laanwj added a commit to laanwj/bitcoin that referenced this pull request Sep 26, 2016
@instagibbs @laanwj instagibbs + laanwj Add basic test for IsStandard witness transaction blinding
Github-Pull: #8525
Rebased-From: ca10a03
b394a96
@laanwj laanwj removed the Needs backport label Sep 26, 2016
@laanwj
Member
laanwj commented Sep 26, 2016

This is backported in #8815, removing tag

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment