Skip to content

Add assume_valid_target_reached: bool to NetRpc::sync_state #4486

Merged
quake merged 1 commit intonervosnetwork:developfrom
eval-exec:exec/sync_state_assume
Jul 1, 2024
Merged

Add assume_valid_target_reached: bool to NetRpc::sync_state #4486
quake merged 1 commit intonervosnetwork:developfrom
eval-exec:exec/sync_state_assume

Conversation

@eval-exec
Copy link
Collaborator

@eval-exec eval-exec commented Jun 19, 2024 •
edited
Loading

What problem does this PR solve?

Issue Number: close #4485

What's Changed:

Related changes

  • Add assume_valid_target_reached: bool and min_chain_work_reached: bool to sync_state RPC
  • let check_assume_valid_target don't reset SyncConfig::assume_valid_target to None if the assume target has found in Snapshot.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code ci-runs-only: [ quick_checks,linters ]

Side effects

  • Performance regression
  • Breaking backward compatibility

Release note 

Title Only: Include only the PR title in the release note.

@eval-exec eval-exec force-pushed the exec/sync_state_assume branch 4 times, most recently from acb471f to 1df9a16 Compare June 20, 2024 22:50
@eval-exec eval-exec changed the title Add assume_valid_target_reached: bool to sync_state RPC Add assume_valid_target_reached: bool to NetRpc::sync_state Jun 20, 2024
@eval-exec eval-exec force-pushed the exec/sync_state_assume branch 2 times, most recently from b7052aa to 03bf43b Compare June 20, 2024 23:05
@yanguoyu yanguoyu mentioned this pull request Jun 21, 2024
Merged
@eval-exec eval-exec marked this pull request as ready for review June 21, 2024 01:53
@eval-exec eval-exec requested a review from a team as a code owner June 21, 2024 01:53
@eval-exec eval-exec requested review from quake and zhangsoledad and removed request for a team June 21, 2024 01:53
@quake
Copy link
Member

quake commented Jun 21, 2024

could you please add min_chain_work field also?

@eval-exec eval-exec marked this pull request as draft June 21, 2024 05:57
@eval-exec eval-exec force-pushed the exec/sync_state_assume branch 2 times, most recently from 82c4cfe to c9a3fdb Compare June 21, 2024 06:54
@eval-exec eval-exec marked this pull request as ready for review June 21, 2024 06:54
@eval-exec eval-exec force-pushed the exec/sync_state_assume branch from c9a3fdb to eceef48 Compare June 21, 2024 06:54
@eval-exec eval-exec marked this pull request as draft June 21, 2024 06:55
@eval-exec eval-exec force-pushed the exec/sync_state_assume branch from eceef48 to 503b3ae Compare June 21, 2024 06:59
@eval-exec eval-exec marked this pull request as ready for review June 21, 2024 07:00
@eval-exec eval-exec force-pushed the exec/sync_state_assume branch from 503b3ae to 3dd58ed Compare June 21, 2024 07:16
@eval-exec eval-exec added the b:rpc Break RPC interface label Jun 21, 2024
@eval-exec eval-exec force-pushed the exec/sync_state_assume branch from 3dd58ed to a2f860d Compare June 21, 2024 07:39
@eval-exec
Copy link
Collaborator Author

eval-exec commented Jun 21, 2024

I think this PR is ready for review.

eval-exec
eval-exec commented Jun 21, 2024
View reviewed changes
rpc/src/module/net.rs
Comment on lines +728 to +732
let min_chain_work = {
let mut min_chain_work_500k_u128: [u8; 16] = [0; 16];
min_chain_work_500k_u128.copy_from_slice(&MIN_CHAIN_WORK_500K.to_le_bytes()[..16]);
u128::from_le_bytes(min_chain_work_500k_u128)
};
Copy link
Collaborator Author

@eval-exec eval-exec Jun 21, 2024

Choose a reason for hiding this comment

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

MIN_CHAIN_WORK is a value of type u256, however, ckb_jsonrpc does not support Uint256, so I need to cast it to u128 first and then convert it to Uint128.

Cc: @quake

@eval-exec eval-exec force-pushed the exec/sync_state_assume branch from a2f860d to 256a86d Compare June 21, 2024 08:56
quake
quake reviewed Jun 26, 2024
View reviewed changes
rpc/README.md Outdated

* `low_time`: [`Uint64`](#type-uint64) - The download scheduler's time analysis data, the low is the 9/10 of the cut-off point, unit ms

* `min_chain_work`: [`Uint128`](#type-uint128) - The CKB hardcoded MIN_CHAIN_WORK_500K
Copy link
Member

@quake quake Jun 26, 2024

Choose a reason for hiding this comment

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

only mainnet use this hardcoded value, this value acts as a security measure to ensure that a node only synchronizes with other node that have a significant amount of computational work invested in them, thus preventing certain types of attacks and ensuring network integrity. suggest to remove the MIN_CHAIN_WORK_500K word in rpc doc and add an explanation for this field.

Copy link
Collaborator Author

@eval-exec eval-exec Jun 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Updated, removed MIN_CHAIN_WORK_500K word from rpc docs.

quake
quake previously approved these changes Jun 26, 2024
View reviewed changes
@eval-exec eval-exec force-pushed the exec/sync_state_assume branch 3 times, most recently from e4979f8 to f3ed2d4 Compare June 28, 2024 08:34
@eval-exec eval-exec force-pushed the exec/sync_state_assume branch from f3ed2d4 to 0e45cc2 Compare June 28, 2024 08:42
quake
quake previously approved these changes Jul 1, 2024
View reviewed changes
@quake quake added this pull request to the merge queue Jul 1, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 1, 2024
@eval-exec
Add assume_valid_target to sync_state RPC
447b05d 
Signed-off-by: Eval EXEC <execvy@gmail.com>
@eval-exec eval-exec force-pushed the exec/sync_state_assume branch from 0e45cc2 to 447b05d Compare July 1, 2024 06:26
@quake quake added this pull request to the merge queue Jul 1, 2024
Merged via the queue into nervosnetwork:develop with commit aca6d8e Jul 1, 2024
@Keith-CY Keith-CY mentioned this pull request Jul 31, 2024
Merged
@gpBlockchain gpBlockchain mentioned this pull request Aug 1, 2024
Open
This was referenced Aug 1, 2024
Closed
Open
@yanguoyu yanguoyu mentioned this pull request Aug 8, 2024
Closed
@devchenyan devchenyan mentioned this pull request Feb 17, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@quake quake quake approved these changes

@zhangsoledad zhangsoledad Awaiting requested review from zhangsoledad zhangsoledad is a code owner automatically assigned from nervosnetwork/ckb-dev

@chenyukang chenyukang Awaiting requested review from chenyukang

@driftluo driftluo Awaiting requested review from driftluo

Assignees

No one assigned

Labels

b:rpc Break RPC interface

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

How to determine if the node is in the "looking for assume-valid-target" stage?

2 participants

@eval-exec @quake