Add pieces verification to farmers. #725

shamil-gadelshin · 2022-07-27T13:49:13Z

This PR aims to add pieces verification to the DSN sync on farmers. It introduces an additional RPC endpoint on the node side to get the records_root.

Original issue: #709

Changes

records_root endpoint for SubspaceRpcApi in the sc-consensus-subspace-rpc crate
new pieces_verification module in the subspace-farmer
RpcClient addition for the records_root in the subspace-farmer

Comments

This is a proof-of-concept, no optimizations like parallel requests, parameter batching were implemented. Please, add them in the PR-comments.
No manual testing was made because of the non-working --dsn-sync flag in the main branch.

Code contributor checklist:

I have reviewed my own changes one more time to spot typos, unintended changes, following project conventions, etc.
I have prepared clean readable history of commits before submitting this PR to make reviewer's life easier
I have tested my changes and/or added corresponding test cases (if relevant)
I understand that any changes to this PR going forward will notify multiple developers and will try to minimize them
I have added sufficient description of changes to make review process easier
This PR is ready for review by developers

farmer: Implement rpc client method.

nazar-pc

Generally in line with my expectations, no major issues

nazar-pc · 2022-07-27T14:34:13Z

crates/sc-consensus-subspace-rpc/src/lib.rs

@@ -95,6 +97,9 @@ pub trait SubspaceRpcApi {

    #[method(name = "subspace_acknowledgeArchivedSegment")]
    async fn acknowledge_archived_segment(&self, segment_index: u64) -> RpcResult<()>;
+
+    #[method(name = "subspace_recordsRoot")]
+    async fn records_root(&self, segment_index: u64) -> RpcResult<Option<Sha256Hash>>;


I think this should take a vector of segment indexes and return a vector of roots to improve efficiency, otherwise there might be too many requests necessary.

Yes, sure. However, it seems vulnerable to a large indexes vector. It makes sense to introduce MAX_SEGMENT_INDEXES_PER_REQUEST const to limit both farmer and node sides.

Certainly. Requester needs to limit number of roots requested or else response will not fit into max size configured in RPC server and response will fail. For public RPC endpoints we also may want to limit this, so yeah, makes sense to have certain limits on both sides.

nazar-pc · 2022-07-27T14:37:37Z

crates/subspace-farmer/src/pieces_verification.rs

+        let segment_index: u64 = piece_index / MERKLE_NUM_LEAVES as u64;
+        let position: u64 = piece_index % MERKLE_NUM_LEAVES as u64;


MERKLE_NUM_LEAVES and RECORD_SIZE should be derived from FarmerProtocolInfo instead, it allows us to have different values in tests and generally make implementation on the library level more flexible. This is the first time these constants are used on the farmer specifically.

Done. Please, verify the new algorithm.

crates/subspace-farmer/src/pieces_verification.rs

crates/subspace-farmer/src/single_plot_farm.rs

crates/subspace-farmer/src/pieces_verification.rs

# Conflicts: # crates/sc-consensus-subspace-rpc/src/lib.rs

i1i1

LGTM

crates/subspace-farmer/src/dsn.rs

crates/sc-consensus-subspace-rpc/src/lib.rs

crates/subspace-farmer/src/single_plot_farm.rs

crates/subspace-farmer/src/dsn/tests.rs

nazar-pc · 2022-07-28T13:09:01Z

crates/subspace-farmer/src/pieces_verification.rs

+    let records_per_segment =
+        farmer_protocol_info.recorded_history_segment_size as usize / PIECE_SIZE;
+    if farmer_protocol_info.record_size.is_zero() || records_per_segment.is_zero() {


It would be preferable to change record_size and recorded_history_segment_size to NonZeroU32, then you don't ever need to check it. At least in FarmerProtocolInfo 🙂

It is always better to make invalid invariants non-representable in the first place. Though I don't think these two were ever zero anywhere in our codebase anyway. And segment size was always bigger than piece size.

crates/subspace-farmer/src/single_plot_farm.rs

nazar-pc · 2022-07-28T13:25:47Z

crates/sc-consensus-subspace-rpc/src/lib.rs

@@ -95,6 +97,9 @@ pub trait SubspaceRpcApi {

    #[method(name = "subspace_acknowledgeArchivedSegment")]
    async fn acknowledge_archived_segment(&self, segment_index: u64) -> RpcResult<()>;
+
+    #[method(name = "subspace_recordsRoot")]
+    async fn records_root(&self, segment_index: u64) -> RpcResult<Option<Sha256Hash>>;


Certainly. Requester needs to limit number of roots requested or else response will not fit into max size configured in RPC server and response will fail. For public RPC endpoints we also may want to limit this, so yeah, makes sense to have certain limits on both sides.

- change u32 to NonZeroU32 for record_size

- add MAX_SEGMENT_INDEXES_PER_REQUEST limit to rpc call on both client and server sides.

shamil-gadelshin · 2022-07-29T16:15:34Z

The latest changes comment:

fixed review comments
I changed the pieces verification formula to the a recommended one (merkle_num_leaves)
added a minor refactoring for rpc_client_* files to reduce files in the farmer crates's root folder
I tested pieces verification locally with node and two farmers. DSN sync and verification worked. However, it became clear that one incorrect piece in a batch could break the sync process for an entire batch in the current simple implementation. It's probably a good idea to create a more complex sync-algorithm with more options like retry and partially successful verification.

# Conflicts: # crates/subspace-farmer/src/single_plot_farm.rs

nazar-pc

Something broke DSN test reliably and needs to be fixed

crates/sc-consensus-subspace-rpc/src/lib.rs

crates/subspace-farmer/src/rpc_client/mod.rs

- refactor rpc_client module in the subspace-farmer - fix error comment in sc-consensus-subspace-rpc - fix broken test

shamil-gadelshin · 2022-08-01T08:53:05Z

Something broke DSN test reliably and needs to be fixed

Fixed.

shamil-gadelshin added 4 commits July 27, 2022 16:34

subspace-rpc: Introduce records_root endpoint.

926372a

farmer: Implement rpc client method.

farmer: Introduce pieces_verification module.

1ea616b

farmer: Implement rpc_client method.

7743d25

farmer: Add pieces verification to DSN sync.

673fb47

shamil-gadelshin added farmer Farming library/app networking Subspace networking (DSN) node Node (service library/node app) labels Jul 27, 2022

shamil-gadelshin self-assigned this Jul 27, 2022

nazar-pc reviewed Jul 27, 2022

View reviewed changes

i1i1 reviewed Jul 27, 2022

View reviewed changes

crates/subspace-farmer/src/pieces_verification.rs Outdated Show resolved Hide resolved

shamil-gadelshin added 5 commits July 27, 2022 20:04

farmer: Fix review comments.

3d901b5

farmer: Refactor verify_pieces_at_blockchain method.

ef4f9cf

subspace-rpc: Add records_roots request batching.

22011d8

Merge branch 'main' into verify-pieces

79b59b2

# Conflicts: # crates/sc-consensus-subspace-rpc/src/lib.rs

farmer: Fix farmer side of the records_root call.

8ec51dc

shamil-gadelshin requested review from nazar-pc and i1i1 July 28, 2022 11:07

i1i1 previously approved these changes Jul 28, 2022

View reviewed changes

crates/subspace-farmer/src/dsn.rs Outdated Show resolved Hide resolved

crates/sc-consensus-subspace-rpc/src/lib.rs Show resolved Hide resolved

crates/subspace-farmer/src/single_plot_farm.rs Show resolved Hide resolved

crates/subspace-farmer/src/single_plot_farm.rs Show resolved Hide resolved

farmer: Fix review comments (tracing errors).

fb1687f

shamil-gadelshin dismissed i1i1’s stale review via fb1687f July 28, 2022 13:23

i1i1 previously approved these changes Jul 28, 2022

View reviewed changes

nazar-pc reviewed Jul 28, 2022

View reviewed changes

shamil-gadelshin added 5 commits July 28, 2022 22:26

farmer: Fix minor review comments.

a34268a

Modify FarmerProtocolInfo structure.

7ead01d

- change u32 to NonZeroU32 for record_size

Add a limit for a RPC records_roots call.

25982e1

- add MAX_SEGMENT_INDEXES_PER_REQUEST limit to rpc call on both client and server sides.

farmer: Fix pieces verifcation formula.

e4ae0ce

farmer: Move rpc_client_* files to a new folder.

7d1cc50

shamil-gadelshin dismissed i1i1’s stale review via 7d1cc50 July 29, 2022 16:15

Merge branch 'main' into verify-pieces

c866ea7

# Conflicts: # crates/subspace-farmer/src/single_plot_farm.rs

shamil-gadelshin marked this pull request as ready for review July 29, 2022 16:19

shamil-gadelshin requested review from rg3l3dr and liuchengxu as code owners July 29, 2022 16:19

shamil-gadelshin requested a review from nazar-pc July 29, 2022 16:19

nazar-pc reviewed Jul 30, 2022

View reviewed changes

crates/sc-consensus-subspace-rpc/src/lib.rs Outdated Show resolved Hide resolved

crates/subspace-farmer/src/rpc_client/mod.rs Outdated Show resolved Hide resolved

Fix review comments.

790f371

- refactor rpc_client module in the subspace-farmer - fix error comment in sc-consensus-subspace-rpc - fix broken test

shamil-gadelshin requested a review from nazar-pc August 1, 2022 08:53

Merge branch 'main' into verify-pieces

8538361

i1i1 approved these changes Aug 1, 2022

View reviewed changes

nazar-pc approved these changes Aug 3, 2022

View reviewed changes

Merge branch 'main' into verify-pieces

7d54abb

shamil-gadelshin enabled auto-merge August 4, 2022 08:13

shamil-gadelshin merged commit dd02054 into main Aug 4, 2022

shamil-gadelshin deleted the verify-pieces branch August 4, 2022 10:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pieces verification to farmers. #725

Add pieces verification to farmers. #725

shamil-gadelshin commented Jul 27, 2022

nazar-pc left a comment

nazar-pc Jul 27, 2022

shamil-gadelshin Jul 28, 2022

nazar-pc Jul 28, 2022

nazar-pc Jul 27, 2022

shamil-gadelshin Jul 28, 2022

i1i1 left a comment

nazar-pc Jul 28, 2022

nazar-pc Jul 28, 2022

shamil-gadelshin commented Jul 29, 2022

nazar-pc left a comment

shamil-gadelshin commented Aug 1, 2022

		let segment_index: u64 = piece_index / MERKLE_NUM_LEAVES as u64;
		let position: u64 = piece_index % MERKLE_NUM_LEAVES as u64;

Add pieces verification to farmers. #725

Add pieces verification to farmers. #725

Conversation

shamil-gadelshin commented Jul 27, 2022

Changes

Comments

Code contributor checklist:

nazar-pc left a comment

Choose a reason for hiding this comment

nazar-pc Jul 27, 2022

Choose a reason for hiding this comment

shamil-gadelshin Jul 28, 2022

Choose a reason for hiding this comment

nazar-pc Jul 28, 2022

Choose a reason for hiding this comment

nazar-pc Jul 27, 2022

Choose a reason for hiding this comment

shamil-gadelshin Jul 28, 2022

Choose a reason for hiding this comment

i1i1 left a comment

Choose a reason for hiding this comment

nazar-pc Jul 28, 2022

Choose a reason for hiding this comment

nazar-pc Jul 28, 2022

Choose a reason for hiding this comment

shamil-gadelshin commented Jul 29, 2022

nazar-pc left a comment

Choose a reason for hiding this comment

shamil-gadelshin commented Aug 1, 2022