refactor: move policy constants to policy #25388

fanquake · 2022-06-16T09:11:46Z

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

glozow

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

Concept ACK

src/validation.cpp

Sjors · 2022-06-16T11:57:52Z

Same question as #25388 (comment), but otherwise ed3a181be921be3cc1cb7ceb3cfa14930c5b59bc 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 · 2022-06-16T14:00:35Z

Could also move EXTRA_DESCENDANT_TX_SIZE_LIMIT.
And EXTRA_DESCENDANT_TX_SIZE_LIMIT as suggested above.

Added.

DrahtBot · 2022-06-16T15:07:44Z

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 · 2022-06-16T16:06:30Z

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 · 2022-06-17T05:53:53Z

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 28487b4f7321351aaf85b13da29bb16a9db0d27b

maflcko · 2022-06-17T12:42:08Z

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 · 2022-06-17T14:15:33Z

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 · 2022-06-17T21:02:58Z

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 · 2022-06-20T02:42:59Z

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.

-BEGIN VERIFY SCRIPT- sed -i -e "s/static const /static constexpr /" src/policy/policy.h -END VERIFY SCRIPT-

fanquake · 2022-06-20T09:30:01Z

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 · 2022-06-20T10:58:34Z

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.

fanquake mentioned this pull request Jun 16, 2022

Move DEFAULT_ANCESTOR_LIMIT to policy/settings #25295

Closed

glozow reviewed Jun 16, 2022

View reviewed changes

darosior reviewed Jun 16, 2022

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

DrahtBot added Refactoring TX fees and policy Validation labels Jun 16, 2022

fanquake force-pushed the 25295_cleanup_all_constants branch from ed3a181 to 28487b4 Compare June 16, 2022 14:00

This was referenced Jun 16, 2022

Support ignoring "opt-in" flag for RBF (aka full RBF) #25373

Closed

Add a -mempoolfullrbf node setting #25353

Merged

This was referenced Jun 16, 2022

[kernel 3a/n] Decouple CTxMemPool from ArgsManager #25290

Merged

Remove LOCKTIME_MEDIAN_TIME_PAST constant #24565

Merged

ariard approved these changes Jun 16, 2022

View reviewed changes

This was referenced Jun 16, 2022

Use int32_t type for most transaction size/weight values #23962

Merged

Sanitize fee options #22044

Closed

darosior reviewed Jun 17, 2022

View reviewed changes

Riahiamirreza reviewed Jun 19, 2022

View reviewed changes

fanquake mentioned this pull request Jun 19, 2022

refactor: Move some default mempool values to policy.h #25415

Closed

w0xlt approved these changes Jun 20, 2022

View reviewed changes

Riahiamirreza approved these changes Jun 20, 2022

View reviewed changes

CAnon and others added 2 commits June 20, 2022 10:02

refactor: Move DEFAULT_ANCESTOR_LIMIT to policy/policy.h

da8d304

refactor: Move DEFAULT_ANCESTOR_SIZE_LIMIT to policy/policy.h

05fc5fd

fanquake added 6 commits June 20, 2022 10:02

refactor: Move DEFAULT_DESCENDANT_LIMIT to policy/policy.h

a34aa4c

refactor: Move DEFAULT_DESCENDANT_SIZE_LIMIT to policy/policy.h

62d56bb

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

01ccfbe

-BEGIN VERIFY SCRIPT- sed -i -e "s/static const /static constexpr /" src/policy/policy.h -END VERIFY SCRIPT-

refactor: use braced initialization in policy/policy.h

39c6036

refactor: move EXTRA_DESCENDANT_TX_SIZE_LIMIT to policy/policy.h

9c94f3b

refactor: move DEFAULT_*_LIMIT assertions from validation to policy

0d8e68d

fanquake force-pushed the 25295_cleanup_all_constants branch from 28487b4 to 0d8e68d Compare June 20, 2022 09:29

w0xlt approved these changes Jun 20, 2022

View reviewed changes

darosior approved these changes Jun 20, 2022

View reviewed changes

laanwj merged commit 57a491b into bitcoin:master Jun 20, 2022

fanquake deleted the 25295_cleanup_all_constants branch June 20, 2022 19:09

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 20, 2022

Merge bitcoin#25388: refactor: move policy constants to policy

cc71565

bitcoin locked and limited conversation to collaborators Jun 20, 2023

refactor: move policy constants to policy #25388

refactor: move policy constants to policy #25388

Uh oh!

Conversation

fanquake commented Jun 16, 2022

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Jun 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Jun 16, 2022

Uh oh!

DrahtBot commented Jun 16, 2022

Conflicts

Uh oh!

laanwj commented Jun 16, 2022

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 17, 2022

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 17, 2022

Uh oh!

glozow commented Jun 17, 2022

Uh oh!

dongcarl commented Jun 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Riahiamirreza left a comment

Choose a reason for hiding this comment

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

w0xlt commented Jun 20, 2022

Uh oh!

fanquake commented Jun 20, 2022

Uh oh!

laanwj commented Jun 20, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Sjors commented Jun 16, 2022 •

edited

Loading

dongcarl commented Jun 17, 2022 •

edited

Loading