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.

Sign up for GitHub

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

Jump to bottom

Add assume_valid_target_reached: bool to NetRpc::sync_state #4486

Merged
merged 1 commit into from
Jul 1, 2024
Merged

Add assume_valid_target_reached: bool to NetRpc::sync_state #4486

merged 1 commit into from
Jul 1, 2024

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 zhangsoledad and quake 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 marked this pull request as draft June 21, 2024 06:55
@eval-exec eval-exec marked this pull request as ready for review June 21, 2024 07:00
@eval-exec eval-exec added the b:rpc Break RPC interface label Jun 21, 2024
@eval-exec
Copy link
Collaborator Author

I think this PR is ready for review.

eval-exec
eval-exec commented Jun 21, 2024
View reviewed changes
rpc/src/module/net.rs Outdated
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

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

quake
quake reviewed Jun 26, 2024
View reviewed changes
rpc/README.md Outdated
@@ -6853,6 +6861,10 @@ The overall chain synchronization state of this local node.

* `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

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
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>
@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
27 of 32 checks passed
@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
Open
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