Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented May 31, 2022

Seems a bit confusing to put policy stuff into validation, so fix that.

Also fix includes via iwyu.

Also fix includes using iwyu
@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 1, 2022

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25290 ([kernel 3a/n] Decouple CTxMemPool from ArgsManager by dongcarl)
  • #25285 (Add AutoFile without ser-type and ser-version and use it where possible by MarcoFalke)
  • #24513 (CChainState -> Chainstate by jamesob)
  • #23076 (Pass CFeeRate and CTxMemPool in-params by reference to const by jonatack)
  • #17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)

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.

Copy link

@ariard ariard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa4068b, the includes move compiles well locally.

} // namespace Consensus

/** Default for -minrelaytxfee, minimum relay fee for transactions */
static const unsigned int DEFAULT_MIN_RELAY_TX_FEE = 1000;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think to move too the following default values validation.h from in policy.h ?

  • DEFAULT_ANCESTOR_LIMIT
  • DEFAULT_ANCESTOR_SIZE_LIMIT
  • DEFAULT_DESCENDANT_LIMIT
  • DEFAULT_DESCENDANT_SIZE_LIMIT
  • DEFAULT_MEMPOOL_EXPIRY

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to review a follow-up, if someone creates one.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK fa4068b. Make sense to move the global variable to policy/settings and the default constant to policy/policy. Ariard points out other constants that could be moved, which seems fine, but it seems like moving the global variable to be with other related global variables is more significant.

I think this variable was just forgotten in 4a75c9d from #15638 which moved other variables because it wasn't used by wallet code

@maflcko maflcko merged commit 2ab4a80 into bitcoin:master Jun 7, 2022
@maflcko maflcko deleted the 2205-fee-setting-🕹 branch June 7, 2022 09:35
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 8, 2022
fa4068b Move minRelayTxFee to policy/settings (MacroFake)

Pull request description:

  Seems a bit confusing to put policy stuff into validation, so fix that.

  Also fix includes via `iwyu`.

ACKs for top commit:
  ariard:
    ACK fa4068b, the includes move compiles well locally.
  ryanofsky:
    Code review ACK fa4068b. Make sense to move the global variable to policy/settings and the default constant to policy/policy. Ariard points out other constants that could be moved, which seems fine, but it seems like moving the global variable to be with other related global variables is more significant.

Tree-SHA512: adf9619002610d1877f3aef0a9e6115fc4c2ad64135a3e5100824c650b560c47f47ac28894c6214a50a7888355252a9f6f7cec98c23a771a1964160ef1ca77de
laanwj added a commit to bitcoin-core/gui that referenced this pull request Jun 20, 2022
0d8e68d refactor: move DEFAULT_*_LIMIT assertions from validation to policy (fanquake)
9c94f3b refactor: move EXTRA_DESCENDANT_TX_SIZE_LIMIT to policy/policy.h (fanquake)
39c6036 refactor: use braced initialization in policy/policy.h (fanquake)
01ccfbe scripted-diff: use static constexpr in policy/policy.h (fanquake)
62d56bb refactor: Move DEFAULT_DESCENDANT_SIZE_LIMIT to policy/policy.h (fanquake)
a34aa4c refactor: Move DEFAULT_DESCENDANT_LIMIT to policy/policy.h (fanquake)
05fc5fd refactor: Move DEFAULT_ANCESTOR_SIZE_LIMIT to policy/policy.h (fanquake)
da8d304 refactor: Move DEFAULT_ANCESTOR_LIMIT to policy/policy.h (CAnon)

Pull request description:

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

ACKs for top commit:
  laanwj:
    Code review ACK 0d8e68d
  w0xlt:
    reACK bitcoin/bitcoin@0d8e68d
  darosior:
    ACK 0d8e68d

Tree-SHA512: 79900b09dc3a8020b5053ec734f462cb6e8184ed2b76e9d8afae7fe5331bbc906daaa42c0f622782797d971aaf5698aa0155511ec1d15582cc7675c271664a8d
@bitcoin bitcoin locked and limited conversation to collaborators Jun 7, 2023
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.

4 participants