Skip to content

GH-686 Limits on rate-pack values#736

Open
dnwiebe wants to merge 21 commits intomasterfrom
GH-686
Open

GH-686 Limits on rate-pack values#736
dnwiebe wants to merge 21 commits intomasterfrom
GH-686

Conversation

@dnwiebe
Copy link
Collaborator

@dnwiebe dnwiebe commented Oct 28, 2025

Note

Medium Risk
Introduces a new persisted config value and bumps DB schema to v12 with a migration, which can affect node startup/migrations and rate-pack validation. Remaining changes are primarily test/integration stability tweaks and small refactors/logging.

Overview
Adds rate-pack limits as a first-class persisted setting. Introduces DEFAULT_RATE_PACK_LIMITS/RatePackLimits, stores rate_pack_limits in the DB on init, and bumps CURRENT_SCHEMA_VERSION to 12 with a new migration_11_to_12 inserting the default limits.

Updates configuration plumbing and tests. PersistentConfiguration gains rate_pack_limits() with strict parsing/ordering checks, the daemon’s ConfigDaoNull now supplies a default, and setup-reporting tests update expected rate-pack values (scaled to larger wei-like numbers).

Stability/refactor cleanup. Multinode tests are made less flaky (timeouts/sleeps, different external test host, clearer mismatch diagnostics), Docker network creation now retries, gossip/dot-graph rendering gets minor API/formatting tweaks, and accountant charging/logging is refactored to compute total_charge once and include it in debug output.

Written by Cursor Bugbot for commit 53cadb4. This will update automatically on new commits. Configure here.

cursor[bot]

This comment was marked as outdated.

Copy link
Collaborator

@czarte czarte left a comment

Choose a reason for hiding this comment

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

I am still uncertain, this will protect the MASQ Foundation network, because if someone want's to set rate-pack below the limit, it is just one change and cargo build away from doing so and network does not even aknowledge this. I woud like to propose: make an debut_handler rule, that if the debuting node is in standard, or originate-only mode, and have rate pack below (or above) the limits, his debut is dropped on the flor

.expect("Internal error: regex needs four captures")
.as_str(),
)
.expect("Internal error: regex must require u64")
Copy link

Choose a reason for hiding this comment

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

Bug: Regex Capture Error Message Mismatch

The parse_capture function's error message "Internal error: regex needs four captures" is incorrect. The RATE_PACK_LIMIT_FORMAT regex actually has 8 capture groups, not 4. This generic message also makes debugging difficult by not indicating which of the 8 expected fields is missing.

Fix in Cursor Fix in Web

@kauri-hero
Copy link
Contributor

Hey @dnwiebe @czarte - I agree with @czarte comments to add this additional checking in the handlers to ensure Nodes don't have rate-packs outside of the defined limits now, and can be done without too much additional work.

This will cover older versions of Node joining network with rates outside the hard-coded limits

one_common_neighbor.public_key().clone(),
another_common_neighbor.public_key().clone(),
])
vec_to_set(vec![subject_real_node.main_public_key().clone(),])
Copy link

Choose a reason for hiding this comment

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

Test assertion change may mask behavioral regression

Medium Severity

The test debut_target_does_not_introduce_already_known_neighbors had its expected assertion changed from expecting [subject_key, one_common_neighbor_key, another_common_neighbor_key] to expecting only [subject_key]. This significantly alters what the test validates. The change was not discussed in PR comments and appears unrelated to the PR's stated purpose of "rate-pack limits". While the new assertion matches the test name better, this could either be fixing an incorrect test or masking a behavioral regression in gossip handling.

Fix in Cursor Fix in Web

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.

Box::new(StandardGossipHandler::new(
&DEFAULT_RATE_PACK_LIMITS,
logger.clone(),
)),
Copy link

Choose a reason for hiding this comment

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

StandardGossipHandler uses default limits instead of configured limits

High Severity

StandardGossipHandler validates rate packs against DEFAULT_RATE_PACK_LIMITS instead of the configured rate_pack_limits from the database. This is inconsistent with DebutHandler and IntroductionHandler, which both use the configured limits. Nodes with rate packs outside configured limits can bypass validation through standard gossip, undermining the PR's core purpose of enforcing rate pack limits on incoming nodes.

Fix in Cursor Fix in Web

@@ -0,0 +1,67 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Collaborator

Choose a reason for hiding this comment

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

put it to git ignore

@@ -1412,6 +1854,58 @@ impl GossipAcceptorReal {
debut_target_node_addr.clone(),
))
}

fn validate_new_version(
Copy link
Collaborator

Choose a reason for hiding this comment

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

hint: what about name: validate_standard_nodes_requirements
reason: in gossip we refering with version to NeighborhoodDatabase version, so it seems bit confusing name for me here

let (mut valid_agrs, mut bans) = so_far;
if &agr.inner.public_key == database.root_key() {
// Shouldn't ever happen; but an evil Node could try it
// valid_agrs.push(agr);
Copy link
Collaborator

Choose a reason for hiding this comment

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

commented out code?

Qualification::Malformed(malefactor) => {
let (public_key_opt, ip_address_opt, earning_wallet_opt) =
match agrs.iter().find(|agr| {
agr.node_addr_opt.as_ref().map(|na| na.ip_addr())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: I would like to see the tests of malefactor claiming our own ip, that this ip is not comming in here

Copy link
Collaborator

Choose a reason for hiding this comment

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

In method extract_malefactors it looks like we are using agr ip in case we malefactor him for claiming our own IP

let (gossip, gossip_source) = make_introduction(2345, 3456);
let dest_root = make_node_record(7878, true);
let mut dest_db = db_from_node(&dest_root);
// These don't count because they're half-only neighbors. Will they be ignored?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Due to Ban and Log message those are ignored. shout the question stay?

return;
}

trace!(self.logger, "Contents: {}", agrs_to_string(agrs.as_slice()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this necessary here or is leftover from dev?

.rate_pack_limits_result(Ok(RatePackLimits::test_default())),
));
subject.data_directory = data_dir;
// subject.data_directory = data_dir;
Copy link
Collaborator

Choose a reason for hiding this comment

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

commented out code

);
subject.data_directory = data_dir.to_path_buf();
subject.connect_database();
// subject.data_directory = data_dir.to_path_buf();
Copy link
Collaborator

Choose a reason for hiding this comment

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

commented out code

}
}

pub struct PersistentConfigurationFactoryTest {
Copy link
Collaborator

Choose a reason for hiding this comment

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

you said, this is Mock, not Test

}
}

impl NodeRecord {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this really necessary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants