Page MenuHomePhabricator

Merge #13527: policy: Remove promiscuousmempoolflags
ClosedPublic

Authored by markblundeberg on Tue, May 21, 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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markblundeberg created this revision.Tue, May 21, 06:14
Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, May 21, 06:14
markblundeberg added inline comments.Tue, May 21, 06:18
test/functional/feature_cltv.py
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.Wed, May 22, 00:07
deadalnix added inline comments.
src/validation.cpp
732 ↗(On Diff #8771)

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

test/functional/feature_cltv.py
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.Wed, May 22, 00:07
markblundeberg added inline comments.Wed, May 22, 02:28
src/validation.cpp
732 ↗(On Diff #8771)

Sounds good :)

test/functional/feature_cltv.py
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.Thu, May 23, 00:21
markblundeberg edited the summary of this revision. (Show Details)
markblundeberg updated this revision to Diff 8815.

rebase onto fixed feature_cltv.py; const-ify flags

markblundeberg marked an inline comment as done.Thu, May 23, 00:21
markblundeberg added inline comments.Thu, May 23, 00:55
test/functional/feature_cltv.py
193 ↗(On Diff #8815)

note- this whitespace is fixed in parent diff now

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