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.

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

chore: records #1902

Closed
wants to merge 3 commits into from
Closed

chore: records #1902

wants to merge 3 commits into from

Conversation

joshuef
Copy link
Contributor

@joshuef joshuef commented Jun 18, 2024

This pull request primarily modifies the sn_client/src/api.rs, sn_networking/src/driver.rs, sn_networking/src/event/kad.rs, sn_networking/src/lib.rs, sn_networking/src/spends.rs, sn_networking/src/transfers.rs, sn_node/src/put_validation.rs, and sn_node/src/replication.rs files, focusing on improving the handling of spend records and double spend attempts in the network. The changes include renaming methods and adding a new configuration option to accumulate spend attempts, which can help the client decide whether to retry an operation.

Method Renaming:

  • get_signed_spend_from_record is renamed to get_solitary_signed_spend_from_record and now returns an error if a double spend attempt is detected. [1] [2] [3] [4]

New Configuration Option:

  • A new configuration option accumulate_spend_attempts is added to GetRecordCfg. This option determines whether to accumulate all double spend attempts and return them, which can help the client decide whether to retry the operation. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13]

Handling of Spend Records:

  • Changes are made to handle spend records and double spend attempts more effectively. This includes changes to the get_valid_spend method and the addition of a new method try_fetch_solitary_spend_from_network. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]

Other Changes:

  • In sn_node/src/put_validation.rs, a comment is added to clarify that at most two spends for the same unique public key are expected.
  • In sn_node/src/replication.rs, a comment is added to explain that the code does not care about how many nodes are storing a record, and that it can error out with SplitRecord and keep retrying so it does not replicate DoubleSpendAttempted spends.

@@ -516,6 +516,7 @@

// if we don't want to retry, throw permanent error
if cfg.retry_strategy.is_none() {
// TODO this is where to decide if we just return accumulated record eg

Check notice

Code scanning / devskim

A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note

Suspicious comment
}

if all_found_spends.is_empty() {
warn!("For record {pretty_key:?} task {query_id:?}, found no spend records");
Copy link
Member

Choose a reason for hiding this comment

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

We'd want to send something back to the sender here. Else the caller would be hanging on the get operation forever.

Something like:

 sender
    .send(Err(GetRecordError::NoSpendsFoundDuringAccumulation))
    .map_err(|_| NetworkError::InternalMsgChannelDropped)?;
}

Thinking about it, can we move the accumulation of spend attempts to a higher level?
We return SplitRecord error here, so we can use that to accumulate the spends? It'd keep the kad get flow clean imo?

@RolandSherwin
Copy link
Member

RolandSherwin commented Jun 18, 2024

EDIT: We already perform this

Also, earlier, when a node sees a double spend during validation, it'd take Vec<SignedSpends>.sort().take(2) and would store that + propagate that to other peers, but currently it is removed from the code.
Would that help here? I feel like currently, in case of a double spend, the state for each node is different, but if we sort,accumulate them, we'd get a consistent state among nodes and seeing if a spend is a double spend is relatively easy?

@joshuef
Copy link
Contributor Author

joshuef commented Jun 18, 2024

Closing in favour of 1904

@joshuef joshuef closed this Jun 18, 2024
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.

2 participants