Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

net processing: clamp PeerManager::Options user input #28149

Merged
merged 3 commits into from
Aug 9, 2023
Merged

net processing: clamp PeerManager::Options user input #28149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aa89e04
doc: document PeerManager::Options members
stickies-v Jul 25, 2023
e451d1e
net processing: clamp -maxorphantx to uint32_t bounds
stickies-v Jul 25, 2023
547fa52
net processing: clamp -blockreconstructionextratxn to uint32_t bounds
stickies-v Jul 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 11 additions & 5 deletions src/net_processing.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,10 @@ class ChainstateManager;
/** Whether transaction reconciliation protocol should be enabled by default. */
static constexpr bool DEFAULT_TXRECONCILIATION_ENABLE{false};
/** Default for -maxorphantx, maximum number of orphan transactions kept in memory */
static const unsigned int DEFAULT_MAX_ORPHAN_TRANSACTIONS = 100;
/** Default number of orphan+recently-replaced txn to keep around for block reconstruction */
static const unsigned int DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN = 100;
static const uint32_t DEFAULT_MAX_ORPHAN_TRANSACTIONS{100};
/** Default number of non-mempool transactions to keep around for block reconstruction. Includes
orphan, replaced, and rejected transactions. */
static const uint32_t DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN{100};
static const bool DEFAULT_PEERBLOOMFILTERS = false;
static const bool DEFAULT_PEERBLOCKFILTERS = false;
/** Threshold for marking a node to be discouraged, e.g. disconnected and added to the discouragement filter. */
Expand All @@ -46,11 +47,16 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
{
public:
struct Options {
/** Whether this node is running in -blocksonly mode */
//! Whether this node is running in -blocksonly mode
bool ignore_incoming_txs{DEFAULT_BLOCKSONLY};
//! Whether transaction reconciliation protocol is enabled
bool reconcile_txs{DEFAULT_TXRECONCILIATION_ENABLE};
//! Maximum number of orphan transactions kept in memory
uint32_t max_orphan_txs{DEFAULT_MAX_ORPHAN_TRANSACTIONS};
size_t max_extra_txs{DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN};
//! Number of non-mempool transactions to keep around for block reconstruction. Includes
//! orphan, replaced, and rejected transactions.
uint32_t max_extra_txs{DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN};
//! Whether all P2P messages are captured to disk
bool capture_messages{false};
};

Expand Down
7 changes: 5 additions & 2 deletions src/node/peerman_args.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,21 @@
#include <common/args.h>
#include <net_processing.h>

#include <algorithm>
#include <limits>

namespace node {

void ApplyArgsManOptions(const ArgsManager& argsman, PeerManager::Options& options)
{
if (auto value{argsman.GetBoolArg("-txreconciliation")}) options.reconcile_txs = *value;

if (auto value{argsman.GetIntArg("-maxorphantx")}) {
options.max_orphan_txs = uint32_t(std::max(int64_t{0}, *value));
options.max_orphan_txs = uint32_t((std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max())));
Copy link
Member

Choose a reason for hiding this comment

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

unrelated: May be good to write a clang-tidy plugin to enforce the limits are compile-time constants and in range to avoid silent UB at runtime?

The in-range one can be submitted to upstream and the other check can be done in this repo.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
options.max_orphan_txs = uint32_t((std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max())));
options.max_orphan_txs = uint32_t(std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max()));

nit, if you re-touch?

}

if (auto value{argsman.GetIntArg("-blockreconstructionextratxn")}) {
options.max_extra_txs = size_t(std::max(int64_t{0}, *value));
options.max_extra_txs = uint32_t((std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max())));
stickies-v marked this conversation as resolved.
Show resolved Hide resolved
}

if (auto value{argsman.GetBoolArg("-capturemessages")}) options.capture_messages = *value;
Expand Down