Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Jun 23, 2018

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.

@maflcko
Copy link
Member Author

maflcko commented Jun 23, 2018

As a side note: The current implementation is broken anyway because it returns a dirty state in some cases, but doesn't fail to add the transaction to the mempool. See explanation in the first comment of #13407 (comment)

@laanwj
Copy link
Member

laanwj commented Jun 24, 2018

Concept ACK.
Though I wonder why this was the case. Does removing this reduce test coverage?

@maflcko
Copy link
Member Author

maflcko commented Jun 24, 2018

I believe this increases overall test coverage.

Clearly by removing (partly untested) code in ATMP the coverage increases there. Though, to the best of my knowledge the runtime error of ("TestBlockValidity failed") in miner is no longer covered. If you think this coverage is important, I am happy to volunteer to write a unit test for this, but I think introducing bugs in other modules just so that we can abuse functional tests for an increase in line coverage is not the way to go.

@sipa
Copy link
Member

sipa commented Jun 27, 2018

@morcos You introduced this in 4f7ff00. Any comments?

@morcos
Copy link
Contributor

morcos commented Jun 27, 2018

I'm a bit rusty on all this code, but in principle I'm fine with no longer having that flag.

Concept ACK

@sipa
Copy link
Member

sipa commented Jul 14, 2018

utACK faa2444. I didn't review the test changes.

@fanquake
Copy link
Member

After merging this there are still a couple mentions of -promiscuousmempoolflags:

"-prematurewitness", "-walletprematurewitness", "-promiscuousmempoolflags", "-blockminsize", "-dbcrashratio", "-forcecompactdb", "-usehd",

SET_DOC_OPTIONAL = set(['-rpcssl', '-benchmark', '-h', '-help', '-socks', '-tor', '-debugnet', '-whitelistalwaysrelay', '-promiscuousmempoolflags', '-blockminsize', '-dbcrashratio', '-forcecompactdb', '-usehd'])

@maflcko
Copy link
Member Author

maflcko commented Jul 15, 2018

@fanquake This is wanted. We could remove those in the version that follows the version which includes this pull request.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 25, 2018

Note to reviewers: This pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@laanwj
Copy link
Member

laanwj commented Aug 7, 2018

utACK 870bd4c

@laanwj laanwj merged commit faa2444 into bitcoin:master Aug 7, 2018
laanwj added a commit that referenced this pull request Aug 7, 2018
faa2444 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.

Tree-SHA512: 3b897aa9604ac8d82ebe9573c6efd468c93ddaa08d378ebc902e247b7aa6c68fcde71e5b449c08f17a067146cdc66dc50a67ce06d07607c27e5189a49c3fba3f
@maflcko maflcko deleted the Mf1806-NoPromiscuousmempool branch August 7, 2018 14:27
maflcko pushed a commit that referenced this pull request Oct 21, 2018
…r(...)

97ddc60 validation: Pass chainparams in AcceptToMemoryPoolWorker(...) (practicalswift)

Pull request description:

  Remove unused `CChainParams` argument in `AcceptToMemoryPoolWorker(...)`.

  After the merge of #13527 ("policy: Remove promiscuousmempoolflags") yesterday the `CChainParams` argument is no longer used in `AcceptToMemoryPoolWorker(...)`.

Tree-SHA512: f1bab4498b64f0ab5230b8172f860df8fa8a302e4ee7385be4ba9c65a37cbc3ef640df78348c477169b9414e5c6a160a0b6471a11f4bb27921500ec208ef5340
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 9, 2019
faa2444 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.

Tree-SHA512: 3b897aa9604ac8d82ebe9573c6efd468c93ddaa08d378ebc902e247b7aa6c68fcde71e5b449c08f17a067146cdc66dc50a67ce06d07607c27e5189a49c3fba3f
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants