refactor: Remove all policy globals #25648

MarcoFalke commented at 1:25 PM on July 20, 2022: member

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)

fanquake added the label Refactoring on Jul 20, 2022

fanquake requested review from glozow on Jul 20, 2022

MarcoFalke force-pushed on Jul 20, 2022

jonatack commented at 4:44 PM on July 20, 2022: contributor

Approach ACK

DrahtBot commented at 5:15 PM on July 20, 2022: contributor

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 commented at 11:34 PM on July 20, 2022: member

Concept ACK

glozow commented at 6:47 AM on July 21, 2022: member

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).

MarcoFalke renamed this:
~~refactor: Remove ::fRequireStandard global~~
refactor: Remove all policy globals
on Jul 21, 2022

MarcoFalke force-pushed on Jul 21, 2022

in src/kernel/mempool_options.h:41 in 77777aee1b outdated

  35 | @@ -33,6 +36,9 @@ struct MemPoolOptions {
  36 |      int check_ratio{0};
  37 |      int64_t max_size_bytes{DEFAULT_MAX_MEMPOOL_SIZE_MB * 1'000'000};
  38 |      std::chrono::seconds expiry{std::chrono::hours{DEFAULT_MEMPOOL_EXPIRY_HOURS}};
  39 | +    CFeeRate incremental_relay_fee{DEFAULT_INCREMENTAL_RELAY_FEE};
  40 | +    /** A fee rate smaller than this is considered zero fee (for relaying, mining and transaction creation) */
  41 | +    CFeeRate min_relay_fee{DEFAULT_MIN_RELAY_TX_FEE};

glozow commented at 10:36 AM on July 21, 2022:

Since you're renaming, I suggest calling them feerates instead of fees. For min relay, incremental, and dust

MarcoFalke commented at 1:24 PM on July 21, 2022:

Thx, done

MarcoFalke force-pushed on Jul 21, 2022

MarcoFalke commented at 5:55 PM on July 21, 2022: member

sorry for scope creep, probably for a future PR

Thx, all done here

darosior commented at 9:32 AM on July 22, 2022: member

Nice! Concept ACK.

in src/mempool_args.h:23 in fad91bf9a9 outdated

  19 | @@ -16,7 +20,7 @@ struct MemPoolOptions;
  20 |   * @param[in]  argsman The ArgsManager in which to check set options.
  21 |   * @param[in,out] mempool_opts The MemPoolOptions to modify according to \p argsman.
  22 |   */
  23 | -void ApplyArgsManOptions(const ArgsManager& argsman, kernel::MemPoolOptions& mempool_opts);
  24 | +std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& argsman, const CChainParams& chainparams, kernel::MemPoolOptions& mempool_opts);

glozow commented at 10:27 AM on July 25, 2022:

nit in fad91bf9a97d62a11a419fda7654107a54454f86

Add documentation to doxygen comment?

MarcoFalke commented at 12:17 PM on July 25, 2022:

Thx, done

in src/validation.cpp:844 in 2222644eb3 outdated

 840 | @@ -840,7 +841,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
 841 |          return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "bad-txns-too-many-sigops",
 842 |                  strprintf("%d", nSigOpsCost));
 843 |  
 844 | -    // No individual transactions are allowed below minRelayTxFee and mempool min fee except from
 845 | +    // No individual transactions are allowed below the min relay fee and mempool min fee except from

glozow commented at 10:33 AM on July 25, 2022:

nit in 2222644eb37da606e1eeabeda78007b687889de6

    // No individual transactions are allowed below the min relay feerate and mempool min feerate except from

MarcoFalke commented at 12:17 PM on July 25, 2022:

Thx, done

in src/test/transaction_tests.cpp:748 in fa3cc7457c outdated

 744 | @@ -745,7 +745,6 @@ BOOST_AUTO_TEST_CASE(test_witness)
 745 |  
 746 |  BOOST_AUTO_TEST_CASE(test_IsStandard)
 747 |  {
 748 | -    LOCK(cs_main);

glozow commented at 10:38 AM on July 25, 2022:

not unused as of fa3cc7457c. probably should squash this commit into fa89b895f3, or move it after?

MarcoFalke commented at 12:12 PM on July 25, 2022:

Can you explain how it is used?

glozow commented at 12:19 PM on July 25, 2022:

oh oops, I thought dustRelayFee needed it for some reason

in src/test/fuzz/tx_pool.cpp:127 in fa3b8a8d7d outdated

 123 |      CTxMemPool::Options mempool_opts{MemPoolOptionsForTest(node)};
 124 |  
 125 |      // ...override specific options for this specific fuzz suite
 126 |      mempool_opts.estimator = nullptr;
 127 |      mempool_opts.check_ratio = 1;
 128 | +    mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool();

glozow commented at 11:18 AM on July 25, 2022:

in fa3b8a8d7dddbb6fcf51ac447acc0b1c16bbde20 Not changed by this PR, but why don't we just fuzz with require standard always on? false just disables some tests, no?

MarcoFalke commented at 12:15 PM on July 25, 2022:

Is there a reason not to fuzz the additional branches?

glozow commented at 9:06 AM on August 2, 2022:

Not strongly opinionated, resolving this comment, but have a question to clarify my understanding. My thinking here was based on the check_ratio set to 1, since any other value just means you run check() less often. AFAICT it's the same idea with standardness checks; if you turn it off, you're running fewer tests. Is it another branch if the codepaths you can hit with true is a superset of the codepaths you can hit with false? Am I misunderstanding?

darosior commented at 9:57 AM on August 2, 2022:

I think it is good to also fuzz for non standard transactions, since we support it. Otherwise the validation codepath for non standard transactions that would be executed in "production" would not be fuzzed. On the other hand, "production" is only testnet here since we don't allow -acceptnonstdtxn for mainnet..

nit: Since this is using cycles for a test-only option, maybe we could lower the probability of setting require_standard to false? Something like require_standard = (ConsumeIntegralInRange(1, 10) % 10 != 0)?

MarcoFalke commented at 10:16 AM on August 2, 2022:

AFAICT it's the same idea with standardness checks;

Not really. check will not mutate the mempool. However, require_standard may leave the mempool in a different state with the same ATMP calls, but a different value of require_standard. For example tx-size may create more inputs in the mempool. Or a raw non-standard script may trigger more paths in script evaluation. I think the mempool also uses script caching, so I was hoping for this to also be a check that the mempool doesn't print coins out of thin air.

glozow commented at 11:13 AM on August 2, 2022:

check will not mutate the mempool. However, require_standard may leave the mempool in a different state with the same ATMP calls, but a different value of require_standar

Ahh yeah, makes sense that it's different branches, thanks!

glozow commented at 11:43 AM on July 25, 2022: member

reviewed up to fac940af80a5e8babc9da79843f324147d8e41f5

MarcoFalke force-pushed on Jul 25, 2022

DrahtBot added the label Needs rebase on Jul 26, 2022

MarcoFalke force-pushed on Jul 26, 2022

DrahtBot removed the label Needs rebase on Jul 26, 2022

in src/mempool_args.cpp:68 in fad51d5dac outdated

  63 | +        }
  64 | +    } else if (mempool_opts.incremental_relay_feerate > mempool_opts.min_relay_feerate) {
  65 | +        // Allow only setting incremental fee to control both
  66 | +        mempool_opts.min_relay_feerate = mempool_opts.incremental_relay_feerate;
  67 | +        LogPrintf("Increasing minrelaytxfee to %s to match incrementalrelayfee\n", mempool_opts.min_relay_feerate.ToString());
  68 | +    }

glozow commented at 9:26 AM on August 2, 2022:

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

in src/script/standard.h:47 in fac85fe063 outdated

  41 | @@ -41,11 +42,11 @@ static const unsigned int MAX_OP_RETURN_RELAY = 83;
  42 |  /**
  43 |   * A data carrying output is an unspendable output containing data. The script
  44 |   * type is designated as TxoutType::NULL_DATA.
  45 | + *
  46 | + * Maximum size of TxoutType::NULL_DATA scripts that this node considers standard.
  47 | + * If nullopt, any size is nonstandard.

glozow commented at 9:26 AM on August 2, 2022:

Agree with the approach of optional<size> in fac85fe0632eace9b49d5bdbc0e9e47765b01090 :+1:. At first was unsure about introducing a global g_max_datacarrier_bytes, then realized it was moved into policy in fa727f139c38e24fa23a6455b0b274177b41747c.

glozow requested review from luke-jr on Aug 2, 2022

glozow requested review from ariard on Aug 2, 2022

glozow requested review from darosior on Aug 2, 2022

glozow requested review from jonatack on Aug 2, 2022

glozow commented at 9:41 AM on August 2, 2022: member

ACK fa727f139c38e24fa23a6455b0b274177b41747c

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

in src/rpc/net.cpp:649 in fad51d5dac outdated

 644 | @@ -645,8 +645,11 @@ static RPCHelpMan getnetworkinfo()
 645 |          obj.pushKV("connections_out", node.connman->GetNodeCount(ConnectionDirection::Out));
 646 |      }
 647 |      obj.pushKV("networks",      GetNetworksInfo());
 648 | -    obj.pushKV("relayfee",      ValueFromAmount(::minRelayTxFee.GetFeePerK()));
 649 | -    obj.pushKV("incrementalfee", ValueFromAmount(::incrementalRelayFee.GetFeePerK()));
 650 | +    if (node.mempool) {
 651 | +        // Those fields can be deprecated, to be replaced by the getmempoolinfo fields

darosior commented at 10:00 AM on August 2, 2022:

Should we open an issue for this?

MarcoFalke commented at 1:03 PM on August 2, 2022:

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.

jonatack commented at 4:47 PM on September 26, 2022:

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).

glozow commented at 4:49 PM on September 26, 2022:

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

in src/script/standard.h:49 in fa727f139c outdated

  43 | - * type is designated as TxoutType::NULL_DATA.
  44 | - *
  45 | - * Maximum size of TxoutType::NULL_DATA scripts that this node considers standard.
  46 | - * If nullopt, any size is nonstandard.
  47 | - */
  48 | -extern std::optional<unsigned> g_max_datacarrier_bytes;

darosior commented at 11:41 AM on August 2, 2022:

nit: nMaxDatacarrierBytes is still mentionned just above in the comment for MAX_OP_RETURN_RELAY.

MarcoFalke commented at 1:33 PM on August 2, 2022:

Thx, fixed.

darosior commented at 12:14 PM on August 2, 2022: member

utACK fa727f139c38e24fa23a6455b0b274177b41747c

Return optional error from ApplyArgsManOptions

Also pass in a (for now unused) reference to the params.

Both changes are needed for the next commit.

fa468bdfb6

Remove ::fRequireStandard global fa148602e6

Remove ::incrementalRelayFee and ::minRelayTxFee globals fa9cba7afb

test: Remove unused cs_main fa7a9114e5

Remove ::IsStandardTx(tx, reason) alias

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

fa8a7f01fe

Remove ::dustRelayFee fadc14e4f5

Remove ::fIsBareMultisigStd global fa2f6c1a61

Remove ::GetVirtualTransactionSize() alias

Each alias is only used in one place.

fa477d32ee

Combine datacarrier globals into one fa2a6b8516

Pass datacarrier setting into IsStandard fad0b4fab8

Remove ::g_max_datacarrier_bytes global 66664384a6

scripted-diff: Move mempool_args to src/node

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-

fac812ca83

sort after scripted-diff ddddd6913b

MarcoFalke force-pushed on Aug 2, 2022

MarcoFalke commented at 1:33 PM on August 2, 2022: member

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

in src/mempool_args.h:24 in fa468bdfb6 outdated

  20 |   *
  21 |   * @param[in]  argsman The ArgsManager in which to check set options.
  22 |   * @param[in,out] mempool_opts The MemPoolOptions to modify according to \p argsman.
  23 |   */
  24 | -void ApplyArgsManOptions(const ArgsManager& argsman, kernel::MemPoolOptions& mempool_opts);
  25 | +[[nodiscard]] std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& argsman, const CChainParams& chainparams, kernel::MemPoolOptions& mempool_opts);

ryanofsky commented at 8:01 PM on August 2, 2022:

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

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

MarcoFalke commented at 7:09 AM on August 3, 2022:

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

in src/test/script_p2sh_tests.cpp:21 in fa8a7f01fe outdated

  17 | @@ -18,6 +18,11 @@
  18 |  #include <boost/test/unit_test.hpp>
  19 |  
  20 |  // Helpers:
  21 | +bool IsStandardTx(const CTransaction& tx, std::string& reason)

ryanofsky commented at 8:11 PM on August 2, 2022:

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

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

MarcoFalke commented at 7:10 AM on August 3, 2022:

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

MarcoFalke commented at 9:22 AM on August 3, 2022:

Fixed in #25772

ryanofsky commented at 8:33 PM on August 2, 2022: contributor

Code review ACK ddddd6913b1bdee1cad89a32d363306ea1f7b8d7

ryanofsky approved

in src/kernel/mempool_options.h:54 in ddddd6913b

  49 | +     * Maximum size of TxoutType::NULL_DATA scripts that this node considers standard.
  50 | +     * If nullopt, any size is nonstandard.
  51 | +     */
  52 | +    std::optional<unsigned> max_datacarrier_bytes{DEFAULT_ACCEPT_DATACARRIER ? std::optional{MAX_OP_RETURN_RELAY} : std::nullopt};
  53 | +    bool permit_bare_multisig{DEFAULT_PERMIT_BAREMULTISIG};
  54 | +    bool require_standard{true};

ariard commented at 10:44 PM on August 2, 2022:

could be nice to document variables

MarcoFalke commented at 7:07 AM on August 3, 2022:

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.

in src/node/mempool_args.cpp:89 in ddddd6913b

  84 | +        mempool_opts.max_datacarrier_bytes = argsman.GetIntArg("-datacarriersize", MAX_OP_RETURN_RELAY);
  85 | +    } else {
  86 | +        mempool_opts.max_datacarrier_bytes = std::nullopt;
  87 | +    }
  88 | +
  89 | +    mempool_opts.require_standard = !argsman.GetBoolArg("-acceptnonstdtxn", !chainparams.RequireStandard());

ariard commented at 11:05 PM on August 2, 2022:

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

MarcoFalke commented at 7:05 AM on August 3, 2022:

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

glozow commented at 8:37 AM on August 3, 2022:

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.

in src/wallet/fees.cpp:90 in ddddd6913b

  86 | @@ -87,7 +87,7 @@ CFeeRate GetDiscardRate(const CWallet& wallet)
  87 |      CFeeRate discard_rate = wallet.chain().estimateSmartFee(highest_target, false /* conservative */);
  88 |      // Don't let discard_rate be greater than longest possible fee estimate if we get a valid fee estimate
  89 |      discard_rate = (discard_rate == CFeeRate(0)) ? wallet.m_discard_rate : std::min(discard_rate, wallet.m_discard_rate);
  90 | -    // Discard rate must be at least dustRelayFee
  91 | +    // Discard rate must be at least dust relay feerate

ariard commented at 11:11 PM on August 2, 2022:

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

MarcoFalke commented at 7:04 AM on August 3, 2022:

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.

ariard approved

ariard commented at 11:12 PM on August 2, 2022: member

Light Code Review ACK ddddd69

glozow commented at 8:43 AM on August 3, 2022: member

re ACK ddddd69

changes since last review:

changed comment from dustRelayFee to dust relay feerate
changed comment from nMaxDatacarrierBytes to -datacarriersize for #25648 (review)
moved mempool_args to src/node and sorted includes

glozow merged this on Aug 3, 2022

glozow closed this on Aug 3, 2022

MarcoFalke deleted the branch on Aug 3, 2022

fanquake referenced this in commit 4a4289e2c9 on Aug 3, 2022

fanquake referenced this in commit 4d13fe47be on May 26, 2023

sidhujag referenced this in commit 20272f34e0 on May 26, 2023

bitcoin locked this on Sep 26, 2023