node: change a tx-relay on/off flag to enum #33567
Conversation
Previously the `bool relay` argument to `BroadcastTransaction()` designated: ``` relay=true: add to the mempool and broadcast to all peers relay=false: add to the mempool ``` Change this to an `enum`, so it is more readable and easier to extend with a 3rd option. Consider these example call sites: ```cpp Paint(true); // Or Paint(/*is_red=*/true); ``` vs ```cpp Paint(RED); ``` The idea for putting `TxBroadcastMethod` into `node/types.h` by Ryan. Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33567. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. LLM Linter (✨ experimental)Possible typos and grammar issues:
drahtbot_id_5_m |
| //! Transaction is added to memory pool, if the transaction fee is below the | ||
| //! amount specified by max_tx_fee, and broadcast to all peers if relay is set to true. | ||
| //! Return false if the transaction could not be added due to the fee or for another reason. | ||
| //! Consume a local transaction, optionally adding it to the mempool and |
There was a problem hiding this comment.
The transaction is passed by const ref, so it's not really "consuming" it. Perhaps "Process a local transaction"?
| * Methods to broadcast a local transaction. | ||
| * Used to influence `BroadcastTransaction()` and its callers. | ||
| */ | ||
| enum TxBroadcastMethod : uint8_t { |
There was a problem hiding this comment.
Probably should use class enums for new things?
| }; | ||
|
|
||
| /** | ||
| * Methods to broadcast a local transaction. |
There was a problem hiding this comment.
This isn't a method
| * Methods to broadcast a local transaction. | |
| * Enum to signal to broadcast a local transaction. |
| switch (broadcast_method) { | ||
| case ADD_TO_MEMPOOL_NO_BROADCAST: | ||
| case ADD_TO_MEMPOOL_AND_BROADCAST_TO_ALL: |
There was a problem hiding this comment.
Maybe I'm confused but why include this switch statement if previously we didnt need to? I don't see any if statement for relay here
| const char* what{""}; | ||
| switch (broadcast_method) { | ||
| case node::ADD_TO_MEMPOOL_AND_BROADCAST_TO_ALL: | ||
| what = "to mempool and for broadcast to peers"; | ||
| break; | ||
| case node::ADD_TO_MEMPOOL_NO_BROADCAST: | ||
| what = "to mempool without broadcast"; | ||
| break; | ||
| } | ||
| WalletLogPrintf("Submitting wtx %s %s\n", wtx.GetHash().ToString(), what); |
There was a problem hiding this comment.
I don't think we need the what variable if it's just used as a string to print directly after the if block
| const char* what{""}; | |
| switch (broadcast_method) { | |
| case node::ADD_TO_MEMPOOL_AND_BROADCAST_TO_ALL: | |
| what = "to mempool and for broadcast to peers"; | |
| break; | |
| case node::ADD_TO_MEMPOOL_NO_BROADCAST: | |
| what = "to mempool without broadcast"; | |
| break; | |
| } | |
| WalletLogPrintf("Submitting wtx %s %s\n", wtx.GetHash().ToString(), what); | |
| switch (broadcast_method) { | |
| case node::ADD_TO_MEMPOOL_AND_BROADCAST_TO_ALL: | |
| WalletLogPrintf("Submitting wtx %s to mempool and for broadcast to peers\n", wtx.GetHash().ToString()); | |
| break; | |
| case node::ADD_TO_MEMPOOL_NO_BROADCAST: | |
| WalletLogPrintf("Submitting wtx %s to mempool without broadcast\n", wtx.GetHash().ToString()); | |
| break; | |
| } | |
6 participants
Previously the
bool relayargument toBroadcastTransaction()designated:Change this to an
enum, so it is more readable and easier to extend with a 3rd option. Consider these example call sites:vs
Paint(RED);The idea for putting
TxBroadcastMethodintonode/types.hby Ryan.This is part of #29415 Broadcast own transactions only via short-lived Tor or I2P connections. Putting it in its own PR to reduce the size of #29415 and because it does not logically depend on the other commits from there.