Skip to content

GH-686 Limits on rate-pack values#736

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

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

Conversation

@dnwiebe
Copy link
Collaborator

@dnwiebe dnwiebe commented Oct 28, 2025

Note

Medium Risk
Introduces a DB schema bump and new persisted config (rate_pack_limits) with a migration and strict parsing/validation, which could affect node startup or upgrades if existing DB/config data is unexpected.

Overview
Adds a new persisted configuration value rate_pack_limits (with DEFAULT_RATE_PACK_LIMITS) and bumps DB schema to v12, including a new 11→12 migration and updates to DB initialization and the daemon’s config DAO defaults.

Extends PersistentConfiguration to read/validate rate_pack_limits (regex parsing + ordering checks) and adds factory/invalid implementations for safer construction.

Improves accounting debug logs to include computed total charges (and refactors charge calculation), and hardens multinode/integration tests (retries for Docker network creation, longer timeouts/sleeps, and updated HTTP test targets/expectations).

Written by Cursor Bugbot for commit 1c482e7. 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

@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

@@ -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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I decided on validate_incoming_agr(); I think that's pretty clear.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Gone.

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

Copy link
Collaborator Author

@dnwiebe dnwiebe Feb 11, 2026

Choose a reason for hiding this comment

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

I don't understand this issue. I'll ask you about it.

Further elaboration: we talked about it and decided that it could be ignored. However, it led us to a piece of dead code that needs to be driven out.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope.

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.

This is the final PR Bugbot will review for you during this billing cycle

Your free Bugbot reviews will reset on March 14

Details

Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.

To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

</map>
</option>
</component>
</project> No newline at end of file
Copy link

Choose a reason for hiding this comment

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

IDE config file committed to repository

Low Severity

The .idea/SweepConfig.xml file appears to be a user-specific IDE configuration for the Sweep AI assistant plugin, containing BYOK provider configs and model names. This is unrelated to the PR's stated purpose of adding rate-pack value limits. While some .idea files are tracked in this repo, IDE plugin configurations with specific model versions (e.g., claude-sonnet-4-5-20250929, gpt-5-2025-08-07) are typically developer-specific and can cause unnecessary merge conflicts.

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.

</map>
</option>
</component>
</project> No newline at end of file
Copy link

Choose a reason for hiding this comment

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

IDE config file accidentally committed in PR

Low Severity

The .idea/SweepConfig.xml file is an IDE-specific configuration for the Sweep AI plugin containing provider model lists (Anthropic, OpenAI, etc.) and auto-approved tool settings. It's unrelated to the rate-pack limits feature and is not covered by the .gitignore, which already selectively ignores other .idea/ files like copilot.*, azure/, and inspectionProfiles/. This file likely slipped in unintentionally alongside the PR changes.

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.

</map>
</option>
</component>
</project> No newline at end of file
Copy link

Choose a reason for hiding this comment

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

IDE config file accidentally committed to repository

Low Severity

The .idea/SweepConfig.xml file is a personal IDE configuration for the Sweep AI assistant plugin, containing BYOK provider API model preferences (e.g., claude-sonnet-4-5-20250929, gpt-5-2025-08-07). This file is not excluded by .gitignore and doesn't belong in the repository. It's developer-specific tooling configuration that other contributors don't need and may cause merge conflicts.

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 2 potential issues.

Bugbot Autofix prepared fixes for both issues found in the latest run.

  • ✅ Fixed: IDE config file accidentally committed to repository
    • Removed .idea/SweepConfig.xml from the repository and added it to .gitignore so IDE-specific AI config is no longer tracked.
  • ✅ Fixed: rate_pack_limits() panics instead of returning errors
    • Updated rate_pack_limits() to use ? on get("rate_pack_limits") so database access failures are returned as Err instead of panicking.

Create PR

Or push these changes by commenting:

@cursor push 8393005af8
Preview (8393005af8)
diff --git a/.gitignore b/.gitignore
--- a/.gitignore
+++ b/.gitignore
@@ -5,6 +5,7 @@
 .idea/azure/
 .idea/inspectionProfiles/Project_Default.xml
 .idea/copilot.*
+.idea/SweepConfig.xml
 
 ### Node
 node_modules

diff --git a/.idea/SweepConfig.xml b/.idea/SweepConfig.xml
deleted file mode 100644
--- a/.idea/SweepConfig.xml
+++ /dev/null
@@ -1,67 +1,0 @@
-<?xml version="1.0" encoding="UTF-8"?>
-<project version="4">
-  <component name="dev.sweep.assistant.components.SweepConfig">
-    <option name="autoApprovedTools">
-      <set>
-        <option value="list_files" />
-        <option value="read_file" />
-        <option value="search_files" />
-        <option value="find_usages" />
-        <option value="get_errors" />
-      </set>
-    </option>
-    <option name="byokProviderConfigs">
-      <map>
-        <entry key="anthropic">
-          <value>
-            <BYOKProviderConfig>
-              <option name="eligibleModels">
-                <list>
-                  <option value="claude-sonnet-4-5-20250929" />
-                  <option value="claude-opus-4-5-20251101" />
-                  <option value="claude-haiku-4-5" />
-                </list>
-              </option>
-            </BYOKProviderConfig>
-          </value>
-        </entry>
-        <entry key="grok">
-          <value>
-            <BYOKProviderConfig>
-              <option name="eligibleModels">
-                <list>
-                  <option value="grok-code-fast-1" />
-                </list>
-              </option>
-            </BYOKProviderConfig>
-          </value>
-        </entry>
-        <entry key="openai">
-          <value>
-            <BYOKProviderConfig>
-              <option name="eligibleModels">
-                <list>
-                  <option value="gpt-5-2025-08-07" />
-                  <option value="gpt-5-codex" />
-                  <option value="gpt-5.1-2025-11-13" />
-                  <option value="gpt-5.2-2025-12-11" />
-                </list>
-              </option>
-            </BYOKProviderConfig>
-          </value>
-        </entry>
-        <entry key="z-ai">
-          <value>
-            <BYOKProviderConfig>
-              <option name="eligibleModels">
-                <list>
-                  <option value="z-ai/glm-4.7" />
-                </list>
-              </option>
-            </BYOKProviderConfig>
-          </value>
-        </entry>
-      </map>
-    </option>
-  </component>
-</project>
\ No newline at end of file

diff --git a/node/src/db_config/persistent_configuration.rs b/node/src/db_config/persistent_configuration.rs
--- a/node/src/db_config/persistent_configuration.rs
+++ b/node/src/db_config/persistent_configuration.rs
@@ -582,14 +582,9 @@
     }
 
     fn rate_pack_limits(&self) -> Result<RatePackLimits, PersistentConfigError> {
-        let limits_string = self
-            .get("rate_pack_limits")
-            .expect(
-                "Required value rate_pack_limits missing from CONFIG table: database is corrupt!",
-            )
-            .expect(
-                "Required value rate_pack_limits is NULL in CONFIG table: database is corrupt!",
-            );
+        let limits_string = self.get("rate_pack_limits")?.expect(
+            "Required value rate_pack_limits is NULL in CONFIG table: database is corrupt!",
+        );
         let captures = RATE_PACK_LIMIT_FORMAT.captures(limits_string.as_str())
             .unwrap_or_else(|| panic!("Syntax error in rate_pack_limits value '{}': should be <LRBR>-<HRBR>|<LRSR>-<HRSR>|<LEBR>-<HEBR>|<LESR>-<HESR> where L is low, H is high, R is routing, E is exit, BR is byte rate, and SR is service rate. All numbers should be in wei.", limits_string));
         let candidate = RatePackLimits::new(
@@ -2589,6 +2584,20 @@
     }
 
     #[test]
+    fn rate_pack_limits_complains_about_database_error() {
+        let config_dao = ConfigDaoMock::new()
+            .get_result(Err(ConfigDaoError::DatabaseError("booga".to_string())));
+        let subject = PersistentConfigurationReal::new(Box::new(config_dao));
+
+        let result = subject.rate_pack_limits();
+
+        assert_eq!(
+            result,
+            Err(PersistentConfigError::DatabaseError("booga".to_string()))
+        );
+    }
+
+    #[test]
     #[should_panic(
         expected = "Required value rate_pack_limits is NULL in CONFIG table: database is corrupt!"
     )]
This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

</map>
</option>
</component>
</project> No newline at end of file
Copy link

Choose a reason for hiding this comment

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

IDE config file accidentally committed to repository

Low Severity

The .idea/SweepConfig.xml file is an IDE-specific AI assistant configuration containing model names for Anthropic, OpenAI, Grok, and other providers. This PR already adds .idea/copilot.* to .gitignore, suggesting intent to exclude AI tool configs, but SweepConfig.xml was not similarly excluded and got committed.

Fix in Cursor Fix in Web

"exit_service_rate",
);
Ok(candidate)
}
Copy link

Choose a reason for hiding this comment

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

rate_pack_limits() panics instead of returning errors

Medium Severity

The rate_pack_limits() method returns Result<RatePackLimits, PersistentConfigError> but uses .expect() and panic!() for all error paths, meaning it can never return Err. This is inconsistent with the sibling rate_pack() method, which properly propagates database errors via ?. One caller in unprivileged_parse_args_configuration.rs has a dead Err branch that can never execute, and a transient database error will crash the node rather than being handled gracefully.

Fix in Cursor Fix in Web

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