refactor: move policy constants to policy

fanquake commented at 9:11 am on June 16, 2022: member

Picks up #25295. Which was a follow up to [a comment in #25254](https://github.com/bitcoin/bitcoin/pull/25254#discussion_r890595318). Moves policy constants from validation.h to policy.h.

in src/validation.cpp:329 in 6e79dda108 outdated

325@@ -326,7 +326,7 @@ void CChainState::MaybeUpdateMempoolForReorg(
326     // previously-confirmed transactions back to the mempool.
327     // UpdateTransactionsFromBlock finds descendants of any transactions in
328     // the disconnectpool that were added back and cleans up the mempool state.
329-    const uint64_t ancestor_count_limit = gArgs.GetIntArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT);
330+    const uint64_t ancestor_count_limit = gArgs.GetIntArg("-limitancestorcount", ::ancestorLimit);

darosior commented at 9:22 am on June 16, 2022:

Maybe naive question: what is the purpose of having an indirection through ancestorLimit (or others) instead of directly using the constants?

MarcoFalke commented at 10:58 am on June 16, 2022:

Right, I don’t get it either why this is changed.

fanquake commented at 1:58 pm on June 16, 2022:

No real reason. Makes more sense to drop just given some of the constants are still used directly. Removed.

glozow commented at 9:26 am on June 16, 2022: member

utACK ed3a181be921be3cc1cb7ceb3cfa14930c5b59bc, lightly reviewed

Could also move EXTRA_DESCENDANT_TX_SIZE_LIMIT. cc @ariard, I think you had some ideas about organizing policy settings?

darosior commented at 9:30 am on June 16, 2022: member

Concept ACK

DrahtBot added the label Refactoring on Jun 16, 2022

DrahtBot added the label TX fees and policy on Jun 16, 2022

DrahtBot added the label Validation on Jun 16, 2022

Sjors commented at 11:57 am on June 16, 2022: member

Same question as #25388 (review), but otherwise ed3a181be921be3cc1cb7ceb3cfa14930c5b59bc lgtm. And maybe move DEFAULT_MEMPOOL_EXPIRY too, see #25295 (review)? And EXTRA_DESCENDANT_TX_SIZE_LIMIT as suggested above. Any others? I didn’t see any obvious ones.

fanquake commented at 2:00 pm on June 16, 2022: member

Could also move EXTRA_DESCENDANT_TX_SIZE_LIMIT. And EXTRA_DESCENDANT_TX_SIZE_LIMIT as suggested above.

Added.

fanquake force-pushed on Jun 16, 2022

DrahtBot commented at 3:07 pm on June 16, 2022: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25373 (Support ignoring “opt-in” flag for RBF (aka full RBF) by luke-jr)
#25353 (Add a -fullrbf node setting by ariard)
#25290 ([kernel 3a/n] Decouple CTxMemPool from ArgsManager by dongcarl)
#24565 (Remove LOCKTIME_MEDIAN_TIME_PAST constant by MarcoFalke)
#23962 (Use int32_t type for transaction size/weight consistently by hebasto)
#22044 (Sanitize fee options by amadeuszpawlik)

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 commented at 4:06 pm on June 16, 2022: member

Concept ACK

ariard approved

ariard commented at 8:31 pm on June 16, 2022: member

Code Review ACK 28487b4

I’ve browsed few more files like txmempool,cpp, txorphanage.cpp, validationinterfaces.cpp to see if we had not other random policy-related variables lost in the wrong garden.

cc @ariard, I think you had some ideas about organizing policy settings?

Sure though, I think it can be carry-on as a follow-up or elsewhere. That PR is already a step forward in term of codebase organization by sorting out policy variables from consensus/validation. It would be nice too in the future to document some missing variables like DUST_RELAY_TX_FEE in policy/mempool-limits.md

MarcoFalke commented at 5:53 am on June 17, 2022: member

Looks like this is 100% conflicting with:

#25290 ([kernel 3a/n] Decouple CTxMemPool from ArgsManager by dongcarl)

So it might be best to figure out which approach should be taken.

darosior commented at 7:06 am on June 17, 2022: member

ACK 28487b4f7321351aaf85b13da29bb16a9db0d27b

MarcoFalke commented at 12:42 pm on June 17, 2022: member

At the least it would be good to wait for a comment by @dongcarl . It seems absurd that we need to rush this change in to move some constants to one file when a later change moves all of them to a different file again.

glozow commented at 2:15 pm on June 17, 2022: member

Looks like this is 100% conflicting with #25290

Gah sorry, I hadn’t seen #25290 when I was reviewing. It makes sense to take these out of validation.{h,cpp} but maybe some are policy and some should be managed by CTxMemPool internally?

dongcarl commented at 9:02 pm on June 17, 2022: member

I think it’s likely fine to move these constants to src/policy/policy.h if they indeed logically belong as part of policy. In #25290 I moved them to mempool_limits.h but that’s just an arbitrary decision based on my preference for cleaner header trees (we shouldn’t need to pull in validation.h just to reference a default constant).

If I were to to build #25290 on top of this PR’s changes (done here), we’d just have mempool_limits.h include src/policy/policy.h, which is not so bad (although I do wish we had a src/policy/constants.h or something, but that doesn’t have to be done in this PR).

One thing we should do in this PR is to move the following static assertions in validation.h into src/policy/package.h since all the constants they’re referencing are now in src/policy. https://github.com/bitcoin/bitcoin/blob/e5df0ba0d97e5f8cfd748f09d6ed77b7bfc45471/src/validation.h#L70-L77

Perhaps we could also followup by making sure that the user doesn’t supply us with limits which violate these assertions if we don’t already do so.

Riahiamirreza commented at 5:34 am on June 19, 2022: contributor

What was the need to change the constants to consexpr?

w0xlt approved

w0xlt commented at 2:21 am on June 20, 2022: contributor

Code Review ACK https://github.com/bitcoin/bitcoin/pull/25388/commits/28487b4f7321351aaf85b13da29bb16a9db0d27b

These changes also optimize the code with constexpr and braced initialization.

w0xlt commented at 2:42 am on June 20, 2022: contributor

What was the need to change the constants to consexpr? @Riahiamirreza it’s an optimization. constexpr specifies that the value of an object, variable or function is evaluated strictly at compile time.

Riahiamirreza approved

refactor: Move DEFAULT_ANCESTOR_LIMIT to policy/policy.h da8d304960

refactor: Move DEFAULT_ANCESTOR_SIZE_LIMIT to policy/policy.h 05fc5fdc13

refactor: Move DEFAULT_DESCENDANT_LIMIT to policy/policy.h a34aa4c187

refactor: Move DEFAULT_DESCENDANT_SIZE_LIMIT to policy/policy.h 62d56bb714

scripted-diff: use static constexpr in policy/policy.h

-BEGIN VERIFY SCRIPT-

sed -i -e "s/static const /static constexpr /" src/policy/policy.h

-END VERIFY SCRIPT-

01ccfbe3aa

refactor: use braced initialization in policy/policy.h 39c6036253

refactor: move EXTRA_DESCENDANT_TX_SIZE_LIMIT to policy/policy.h 9c94f3b3a7

refactor: move DEFAULT_*_LIMIT assertions from validation to policy 0d8e68d705

fanquake force-pushed on Jun 20, 2022

fanquake commented at 9:30 am on June 20, 2022: member

One thing we should do in this PR is to move the following static assertions in validation.h into src/policy/package.h since all the constants they’re referencing are now in src/policy.

Added a commit to move these into package.h.

laanwj commented at 10:58 am on June 20, 2022: member

Code review ACK 0d8e68d7054466925ecd165a871fc0e3f53cdafa

w0xlt approved

w0xlt commented at 11:14 am on June 20, 2022: contributor

reACK https://github.com/bitcoin/bitcoin/pull/25388/commits/0d8e68d7054466925ecd165a871fc0e3f53cdafa

darosior approved

darosior commented at 5:29 pm on June 20, 2022: member

ACK 0d8e68d7054466925ecd165a871fc0e3f53cdafa

I think it’s likely fine to move these constants to src/policy/policy.h if they indeed logically belong as part of policy

In my opinion they do.

laanwj merged this on Jun 20, 2022

laanwj closed this on Jun 20, 2022

fanquake deleted the branch on Jun 20, 2022

sidhujag referenced this in commit cc71565893 on Jun 20, 2022

DrahtBot locked this on Jun 20, 2023

refactor: move policy constants to policy #25388

Conflicts