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

fix malleability check in mempool #470

Merged
merged 2 commits into from
Feb 15, 2019
Merged

Conversation

nodech
Copy link
Member

@nodech nodech commented May 27, 2018

Checks in verify in this block are related to witness malleability.
Discussion: bitcoin/bitcoin#8279

// SCRIPT_VERIFY_CLEANSTACK requires SCRIPT_VERIFY_WITNESS, so we
// need to turn both off, and compare against just turning off CLEANSTACK
// to see if the failure is specifically due to witness validation.
-- Bitcoin Core reference https://github.com/bitcoin/bitcoin/blob/5039e4b61beb937bad33ac4300cc784642782589/src/validation.cpp#L906

Summary of the issue:
We may have received witness transaction from old node (without witness) or even with malleated witness, but we don't want to reject those transactions in the mempool. But we still want to detect non standard transactions, that are non-witness.

In case of witness transaction, verifyResult will fail because there is no witness. But if there's no error with VERIFY_WITNESS, it means we have CLEANSTACK issue and transaction is not standard one.

Without this check, it would not reject non-standard P2SH from the mempool reject cache.

Copy link
Member

@tuxcanfly tuxcanfly left a comment

Choose a reason for hiding this comment

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

Reads correct.

@braydonf
Copy link
Contributor

Having a test case for this could be helpful.

@codecov-io

This comment has been minimized.

@nodech
Copy link
Member Author

nodech commented Feb 14, 2019

Added test case and description of the issue.

@braydonf
Copy link
Contributor

Regarding the "... DoS risk in segwit due to malleated transactions" issue (bitcoin/bitcoin#8279), it looks like there were tests added here 72597c9 to check that malleated witness transactions (without witness) are not added to the reject cache (as resolved similarly by bitcoin/bitcoin#8525).

@nodech
Copy link
Member Author

nodech commented Feb 14, 2019

Yes, these tests are still there but still missed this case. This bug was introduced on refactor 01f21b0

@braydonf
Copy link
Contributor

Okay, yep.

Before refactor 01f21b0:

> let flags1 = bcoin.Script.flags.STANDARD_VERIFY_FLAGS;
131039
> let flags2 = flags1 & ~(bcoin.Script.flags.VERIFY_WITNESS | bcoin.Script.flags.VERIFY_CLEANSTACK);
128735
> let flags3 = flags1 & ~bcoin.Script.flags.VERIFY_CLEANSTACK;
130783

After refactor 01f21b0:

> let flags = bcoin.Script.flags.STANDARD_VERIFY_FLAGS;
131039
> flags &= ~bcoin.Script.flags.VERIFY_WITNESS;
128991
> flags &= ~bcoin.Script.flags.VERIFY_CLEANSTACK;
128735
> flags |= bcoin.Script.flags.VERIFY_CLEANSTACK;
128991

With this PR, goes back to before the refactor:

> let flags = bcoin.Script.flags.STANDARD_VERIFY_FLAGS;
131039
> flags &= ~bcoin.Script.flags.VERIFY_WITNESS;
128991
> flags &= ~bcoin.Script.flags.VERIFY_CLEANSTACK;
128735
> flags |= bcoin.Script.flags.VERIFY_WITNESS;
130783

@braydonf braydonf merged commit f0023cb into bcoin-org:master Feb 15, 2019
braydonf added a commit that referenced this pull request Feb 15, 2019
@nodech nodech deleted the fix/reject-cache branch June 28, 2019 08:26
@braydonf braydonf added this to the 2.0.0 milestone Jan 6, 2020
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.

4 participants