Page MenuHomePhabricator

Merge #13527: policy: Remove promiscuousmempoolflags
ClosedPublic

Authored by markblundeberg on May 21 2019, 06:14.

Details

Summary

PR13527 backport https://github.com/bitcoin/bitcoin/pull/13527/files
faa24441ec047ec336b86f586016b9d318c1c0ad policy: Remove promiscuousmempoolflags (MarcoFalke)

Pull request description:

It seems odd to clutter validation code with features that can only ever be used for testing (testnet or regtest). Removing that test-only code makes the mempool logic less painful to understand and easier to reason about when changed or refactored in the future.

ABC note:

  • promiscuousmempoolflags was originally added for segwit tests (#8149).
  • adjusted dersig failure message -> mandatory
  • due to large changes in D2120, the feature_cltv.py changes have been massaged into the appropriate place

Depends on D3079 and D3095

Test Plan

make check
test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
promisc
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5920
Build 9900: Bitcoin ABC Buildbot (legacy)
Build 9899: arc lint + arc unit

Event Timeline

test/functional/feature_cltv.py
180

I notice there is some bizarre whitespace character here marked with ! ... wonder how that happened.

deadalnix requested changes to this revision.May 22 2019, 00:07
deadalnix added inline comments.
src/validation.cpp
732

You can probably merge this with what precedes and make it const while you are at it.

test/functional/feature_cltv.py
156

It does looks like this is the test to be replaced.

180

Probably a misencoded character.

This revision now requires changes to proceed.May 22 2019, 00:07
src/validation.cpp
732

Sounds good :)

test/functional/feature_cltv.py
156

Yeah I thought so at first, but spendtx here is misnamed -- really should be called "fundtx" since the CLTV violation is in the scriptPubKey. The actual tx that executes the bad CLTV is then rejectedtx below. Confusing!

The Core version of this (like we had prior to D2120) has it in the scriptSig of this transaction, so the spendtx name made more sense.

Maybe a new diff should be opened to clarify this stuff, it confused me for a while. I guess the comment here is completely wrong too!

markblundeberg marked an inline comment as done.
markblundeberg edited the summary of this revision. (Show Details)

rebase onto fixed feature_cltv.py; const-ify flags

test/functional/feature_cltv.py
193 ↗(On Diff #8815)

note- this whitespace is fixed in parent diff now

This revision is now accepted and ready to land.May 23 2019, 15:29