Page MenuHomePhabricator

Merge #13527: policy: Remove promiscuousmempoolflags

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



PR13527 backport
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 changes have been massaged into the appropriate place

Depends on D3079 and D3095

Test Plan

make check

Diff Detail

rABC Bitcoin ABC
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markblundeberg created this revision.May 21 2019, 06:14
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 21 2019, 06:14
markblundeberg added inline comments.May 21 2019, 06:18
180 ↗(On Diff #8771)

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.
732 ↗(On Diff #8771)

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

156 ↗(On Diff #8771)

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

180 ↗(On Diff #8771)

Probably a misencoded character.

This revision now requires changes to proceed.May 22 2019, 00:07
markblundeberg added inline comments.May 22 2019, 02:28
732 ↗(On Diff #8771)

Sounds good :)

156 ↗(On Diff #8771)

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; const-ify flags

markblundeberg marked an inline comment as done.May 23 2019, 00:21
markblundeberg added inline comments.May 23 2019, 00:55
193 ↗(On Diff #8815)

note- this whitespace is fixed in parent diff now

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