refactor: move policy constants to policy

[fanquake]

Picks up #25295. Which was a follow up to a comment in #25254.
Moves policy constants from validation.h to policy.h.

[glozow]

utACK ed3a181, lightly reviewed

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

[darosior]

Concept ACK

[Sjors]

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

[fanquake]

Could also move EXTRA_DESCENDANT_TX_SIZE_LIMIT.
And EXTRA_DESCENDANT_TX_SIZE_LIMIT as suggested above.

Added.

[DrahtBot]

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]

Concept ACK

[ariard]

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

[maflcko]

Looks like this is 100% conflicting with:

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

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

[darosior]

ACK 28487b4

[maflcko]

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]

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]

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.

bitcoin/src/validation.h

Lines 70 to 77 in e5df0ba

	// If a package is submitted, it must be within the mempool's ancestor/descendant limits. Since a
	// submitted package must be child-with-unconfirmed-parents (all of the transactions are an ancestor
	// of the child), package limits are ultimately bounded by mempool package limits. Ensure that the
	// defaults reflect this constraint.
	static_assert(DEFAULT_DESCENDANT_LIMIT >= MAX_PACKAGE_COUNT);
	static_assert(DEFAULT_ANCESTOR_LIMIT >= MAX_PACKAGE_COUNT);
	static_assert(DEFAULT_ANCESTOR_SIZE_LIMIT >= MAX_PACKAGE_SIZE);
	static_assert(DEFAULT_DESCENDANT_SIZE_LIMIT >= MAX_PACKAGE_SIZE);

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]

What was the need to change the constants to consexpr?

[w0xlt]

Code Review ACK 28487b4

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

[w0xlt]

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.

[fanquake]

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]

Code review ACK 0d8e68d

[w0xlt]

reACK 0d8e68d

[darosior]

ACK 0d8e68d

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.