refactor: Remove all policy globals #25648

maflcko · 2022-07-20T13:25:03Z

This change is good because:

It moves module-specific init-logic out of the bloated init.cpp
It removes a global from validation.cpp and places it into the data structure that needs it (mempool)

jonatack · 2022-07-20T16:44:19Z

Approach ACK

DrahtBot · 2022-07-20T17:15:11Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25704 (refactor: Remove all validation option globals by MarcoFalke)
#25577 (mempool, refactor: add MemPoolBypass by w0xlt)
#25097 (test: Unit tests for taproot/tapscript coverage in interpreter.cpp by david-bakin)
#24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)
#24007 ([mempool] allow tx replacement by smaller witness by LarryRuane)
#17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)
#13525 (policy: Report reason inputs are nonstandard from AreInputsStandard by Empact)

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.

luke-jr · 2022-07-20T23:34:21Z

Concept ACK

glozow

Approach ACK

MemPoolOptions seems like the correct place to put these things since full_rbf is already there.

I would put ::incrementalRelayFee, ::minRelayFee, dustRelayFee, fIsBareMultisigStd in the same boat (sorry for scope creep, probably for a future PR).

src/kernel/mempool_options.h

maflcko · 2022-07-21T17:55:47Z

sorry for scope creep, probably for a future PR

Thx, all done here

darosior

Nice! Concept ACK.

glozow

reviewed up to fac940af80a5e8babc9da79843f324147d8e41f5

src/mempool_args.h

src/validation.cpp

src/test/transaction_tests.cpp

src/test/fuzz/tx_pool.cpp

glozow

ACK fa727f139c38e24fa23a6455b0b274177b41747c

Lightly reviewed and tested each commit, nice move and cleanup. Would like at least another reviewer to take a look.

src/test/fuzz/tx_pool.cpp

glozow · 2022-08-02T09:26:15Z

src/mempool_args.cpp

in fad51d5dac5b1da1540dd233c6bd096cb3e4a2a5, not changed by this PR, but interesting to note that incremental relay feerate must be set before min relay feerate.

glozow · 2022-08-02T09:26:58Z

src/script/standard.h

Agree with the approach of optional in fac85fe0632eace9b49d5bdbc0e9e47765b01090 👍. At first was unsure about introducing a global g_max_datacarrier_bytes, then realized it was moved into policy in fa727f139c38e24fa23a6455b0b274177b41747c.

darosior

utACK fa727f139c38e24fa23a6455b0b274177b41747c

darosior · 2022-08-02T10:00:06Z

src/rpc/net.cpp

Should we open an issue for this?

Personally I think it is fine to keep them forever, unless there is a strong enough reason to remove them. But this seems to be subjective.

Wondered at first look if launching bitcoind with -blocksonly would make these fields optional but doesn't appear to be the case (and checked that bitcoin-cli -rpcwait getnetworkinfo on a -blocksonly startup returns the fields).

Note that -blocksonly does not mean your node doesn't create a mempool.

src/script/standard.h

Also pass in a (for now unused) reference to the params. Both changes are needed for the next commit.

Apart from tests, it is only used in one place, so there is no need for an alias.

Each alias is only used in one place.

It is part of the node library. Also, it won't be moved to the kernel lib, as it will be pruned of ArgsManager. -BEGIN VERIFY SCRIPT- # Move module git mv src/mempool_args.cpp src/node/ git mv src/mempool_args.h src/node/ # Replacements sed -i 's:mempool_args\.h:node/mempool_args.h:g' $(git grep -l mempool_args) sed -i 's:mempool_args\.cpp:node/mempool_args.cpp:g' $(git grep -l mempool_args) sed -i 's:MEMPOOL_ARGS_H:NODE_MEMPOOL_ARGS_H:g' $(git grep -l MEMPOOL_ARGS_H) -END VERIFY SCRIPT-

maflcko · 2022-08-02T13:33:49Z

Fixed doc typos and added two commits (let me know if I should drop them)

ryanofsky

Code review ACK ddddd69

ryanofsky · 2022-08-02T20:01:37Z

src/mempool_args.h

 * @param[in,out] mempool_opts The MemPoolOptions to modify according to \p argsman.
 */
-void ApplyArgsManOptions(const ArgsManager& argsman, kernel::MemPoolOptions& mempool_opts);
+[[nodiscard]] std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& argsman, const CChainParams& chainparams, kernel::MemPoolOptions& mempool_opts);


In commit "Return optional error from ApplyArgsManOptions" (fa468bd)

This seems good for now, but after #25665 this could return util::Result<void> instead of std::optional<bilingual_str>

Yes, there are also a few other places. See git grep 'std::optional<bilingual_str>'.

ryanofsky · 2022-08-02T20:11:55Z

src/test/script_p2sh_tests.cpp

 #include <boost/test/unit_test.hpp>

 // Helpers:
+bool IsStandardTx(const CTransaction& tx, std::string& reason)


In commit "Remove ::IsStandardTx(tx, reason) alias" (fa8a7f0)

Would be good to declare this static to make sure nobody outside this file can call this

Good point. Will do on the next push or as a follow-up.

Fixed in #25772

ariard

Light Code Review ACK ddddd69

ariard · 2022-08-02T22:44:06Z

src/kernel/mempool_options.h

+     */
+    std::optional<unsigned> max_datacarrier_bytes{DEFAULT_ACCEPT_DATACARRIER ? std::optional{MAX_OP_RETURN_RELAY} : std::nullopt};
+    bool permit_bare_multisig{DEFAULT_PERMIT_BAREMULTISIG};
+    bool require_standard{true};


could be nice to document variables

All options are documented in the manpage. This one under -acceptnonstdtxn. Happy to review a docs pull if there is need for one and someone creates one.

ariard · 2022-08-02T23:05:16Z

src/node/mempool_args.cpp

+        mempool_opts.max_datacarrier_bytes = std::nullopt;
+    }
+
+    mempool_opts.require_standard = !argsman.GetBoolArg("-acceptnonstdtxn", !chainparams.RequireStandard());


could be called require_txstandard we might have standard package in the future, good to dissociate

require_standard is applied to both txs and packages, so I think the current name is fine, no?

I also think package standardness would be included with -acceptnonstdtxn anyway, can't really think of a reason not to. Feel free to reopen if something changes, we can cross this bridge if/when we come to it.

ariard · 2022-08-02T23:11:10Z

src/wallet/fees.cpp

    // Don't let discard_rate be greater than longest possible fee estimate if we get a valid fee estimate
    discard_rate = (discard_rate == CFeeRate(0)) ? wallet.m_discard_rate : std::min(discard_rate, wallet.m_discard_rate);
-    // Discard rate must be at least dustRelayFee
+    // Discard rate must be at least dust relay feerate


m_dust_relay_feerate we do so for other variables doc refs make the code more greppable

wallet.m_dust_relay_feerate doesn't exist, only wallet.chain().relayDustFee(). Happy to switch to relayDustFee or similar, or just remove the comment completely, as the code is self-explanatory. Just let me know.

glozow · 2022-08-03T08:43:45Z

re ACK ddddd69

changes since last review:

changed comment from dustRelayFee to dust relay feerate
changed comment from nMaxDatacarrierBytes to -datacarriersize for refactor: Remove all policy globals #25648 (comment)
moved mempool_args to src/node and sorted includes

… helper fad5bc4 test: Add missing static to IsStandardTx helper (MacroFake) Pull request description: Requested in bitcoin/bitcoin#25648 (comment) Also remove line break from the other two helpers. ACKs for top commit: glozow: utACK fad5bc4 aureleoules: ACK fad5bc4. theStack: ACK fad5bc4 Tree-SHA512: 771411e1fb5939a58491ecf719e1929ab0150b0faae2078ac72bd13117f1d4dcffdeed5027bfae53e4336af25a4f1db47d564abc06a5a2c9ec006a9f67bae104

…al_str>` with `util::Result` 8aa8f73 refactor: Replace std::optional<bilingual_str> with util::Result (Ryan Ofsky) 5f49cb1 util: Add void support to util::Result (MarcoFalke) Pull request description: Replace uses of `std::optional<bilingual_str>` with `util::Result` as suggested bitcoin/bitcoin#25648 (comment), bitcoin/bitcoin#27632 (comment), bitcoin/bitcoin#27632 (comment), bitcoin/bitcoin#24313 (comment) ACKs for top commit: MarcoFalke: very nice ACK 8aa8f73 🏏 TheCharlatan: ACK 8aa8f73 hebasto: ACK 8aa8f73, I have reviewed the code and it looks OK. Tree-SHA512: 6c2f218bc445184ce93ec2b907e61666a05f26f2c9447f69fcb504aa8291b0c693d913f659dfd19813a9e24615546c72cbe2ca419218fd867ff0694c4a1b6a30

… with `util::Result` 8aa8f73 refactor: Replace std::optional<bilingual_str> with util::Result (Ryan Ofsky) 5f49cb1 util: Add void support to util::Result (MarcoFalke) Pull request description: Replace uses of `std::optional<bilingual_str>` with `util::Result` as suggested bitcoin#25648 (comment), bitcoin#27632 (comment), bitcoin#27632 (comment), bitcoin#24313 (comment) ACKs for top commit: MarcoFalke: very nice ACK 8aa8f73 🏏 TheCharlatan: ACK 8aa8f73 hebasto: ACK 8aa8f73, I have reviewed the code and it looks OK. Tree-SHA512: 6c2f218bc445184ce93ec2b907e61666a05f26f2c9447f69fcb504aa8291b0c693d913f659dfd19813a9e24615546c72cbe2ca419218fd867ff0694c4a1b6a30

fanquake added the Refactoring label Jul 20, 2022

fanquake requested a review from glozow July 20, 2022 15:50

maflcko force-pushed the 2207-burn-globals- branch from fa614d3 to fa3b8a8 Compare July 20, 2022 16:03

DrahtBot mentioned this pull request Jul 20, 2022

Replace mempoolfullrbf with more compatible and flexible mempoolreplacement (plus more tests) #25626

Closed

DrahtBot mentioned this pull request Jul 21, 2022

policy: Report reason inputs are nonstandard from AreInputsStandard #13525

Closed

glozow reviewed Jul 21, 2022

View reviewed changes

maflcko changed the title ~~refactor: Remove ::fRequireStandard global~~ refactor: Remove all policy globals Jul 21, 2022

maflcko force-pushed the 2207-burn-globals- branch 4 times, most recently from c29eb24 to fa775db Compare July 21, 2022 12:03

glozow reviewed Jul 21, 2022

View reviewed changes

src/kernel/mempool_options.h Outdated Show resolved Hide resolved

maflcko force-pushed the 2207-burn-globals- branch 4 times, most recently from fa97170 to fa642b5 Compare July 21, 2022 16:55

darosior reviewed Jul 22, 2022

View reviewed changes

This was referenced Jul 24, 2022

fuzz: Remove no-op SetMempoolConstraints #25689

Merged

mempool, refactor: add MemPoolBypass #25577

Closed

glozow reviewed Jul 25, 2022

View reviewed changes

src/mempool_args.h Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/test/transaction_tests.cpp Outdated Show resolved Hide resolved

src/test/fuzz/tx_pool.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the 2207-burn-globals- branch from fa10c83 to 721df35 Compare July 25, 2022 12:16

DrahtBot added the Needs rebase label Jul 26, 2022

glozow reviewed Aug 2, 2022

View reviewed changes

darosior reviewed Aug 2, 2022

View reviewed changes

MacroFake added 13 commits August 2, 2022 15:21

Return optional error from ApplyArgsManOptions

fa468bd

Also pass in a (for now unused) reference to the params. Both changes are needed for the next commit.

Remove ::fRequireStandard global

fa14860

Remove ::incrementalRelayFee and ::minRelayTxFee globals

fa9cba7

test: Remove unused cs_main

fa7a911

Remove ::IsStandardTx(tx, reason) alias

fa8a7f0

Apart from tests, it is only used in one place, so there is no need for an alias.

Remove ::dustRelayFee

fadc14e

Remove ::fIsBareMultisigStd global

fa2f6c1

Remove ::GetVirtualTransactionSize() alias

fa477d3

Each alias is only used in one place.

Combine datacarrier globals into one

fa2a6b8

Pass datacarrier setting into IsStandard

fad0b4f

Remove ::g_max_datacarrier_bytes global

6666438

sort after scripted-diff

ddddd69

maflcko force-pushed the 2207-burn-globals- branch from fa727f1 to ddddd69 Compare August 2, 2022 13:31

ryanofsky reviewed Aug 2, 2022

View reviewed changes

ryanofsky approved these changes Aug 2, 2022

View reviewed changes

ariard approved these changes Aug 2, 2022

View reviewed changes

glozow merged commit f6fdedf into bitcoin:master Aug 3, 2022

maflcko deleted the 2207-burn-globals-🛶 branch August 3, 2022 09:14

maflcko mentioned this pull request Aug 3, 2022

test: Add missing static to IsStandardTx helper #25772

Merged

ryanofsky mentioned this pull request Sep 1, 2022

refactor: Replace std::optional<bilingual_str> with util::Result #25977

Merged

ryanofsky mentioned this pull request May 24, 2023

Improve display address handling for external signer #24313

Merged

bitcoin locked and limited conversation to collaborators Sep 26, 2023

refactor: Remove all policy globals #25648

refactor: Remove all policy globals #25648

Uh oh!

Conversation

maflcko commented Jul 20, 2022

Uh oh!

jonatack commented Jul 20, 2022

Uh oh!

DrahtBot commented Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

luke-jr commented Jul 20, 2022

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Jul 21, 2022

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Aug 2, 2022

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jul 20, 2022 •

edited

Loading