Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

grandpa: rewrite warp sync proof generation #8148

Merged
15 commits merged into from
Feb 25, 2021
Merged

Conversation

andresilva
Copy link
Contributor

@andresilva andresilva commented Feb 18, 2021

This PR rewrites the existing code for warp sync proof generation. After #7339 we now have a way of figuring out the last block for every authority set, this block should contain a digest that signals the next set and most importantly we must have stored a justification for its finalization. By iterating through this index we can easily build a warp sync proof without having to search random blocks on the DB for justifications.

I made a basic benchmark on Kusama for this:

begin: 0x4e532f7e91eac1ac3d26f10cff93c581abc3582a32b98263a257f06d13408d0a #6000000
best: 0xcfaa69a20c2f230e8547bd41afa1aa93c2d74d8a8007a6a64a5a13ac7388a07e #6261998
Warp sync proof generation for 169 transitions took: 123ms

polkadot companion: paritytech/polkadot#2523

@andresilva andresilva added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Feb 18, 2021
@andresilva
Copy link
Contributor Author

andresilva commented Feb 18, 2021

The current interface just returns a warp sync proof which could be limited due to:

  1. Max fragments being reached (currently 256 but we should probably use max message encoding size instead);
  2. A forced change being encountered

For 1) it probably makes sense that we somehow note in the response that we can still prove more and that the client should then request for another proof with a higher begin block.

For 2) we really have to defer to the client regarding how to proceed. If it's a light client it could keep importing blocks until it applies the forced change and then ask for another warp sync proof. When we do a forced change we are essentially hard forking the GRANDPA authority set, so any security assumptions regarding the authority set handoffs are broken. On production chains forced changes should not happen often so ideally the light client would just start from a checkpoint that is past any forced change.

Edit: to make 2) more clear:

S1 ----- S2 ----- S3 ----- F1 ----- S4 ----- S5 -----
#10      #20      #30      #40      #50      #60

S is a regular/standard authority set change, while F is a forced authority set change. When asked for a warp sync proof beginning at any block < #10 the code will produce a proof that will warp us to #30, i.e. it will include proofs for standard changes S1, S2 and S3. After the light client imports F1 by it's own it can then request a warp sync proof starting at #40 which should yield S4 and S5.

@andresilva
Copy link
Contributor Author

I will still add some tests and code to verify the warp sync proof, but this should be good for some basic testing with smoldot :)

Copy link
Contributor

@expenses expenses left a comment

Choose a reason for hiding this comment

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

A small nitpick but otherwise looks good!

client/finality-grandpa-warp-sync/src/proof.rs Outdated Show resolved Hide resolved
client/finality-grandpa/src/finality_proof.rs Show resolved Hide resolved
@expenses
Copy link
Contributor

expenses commented Feb 23, 2021

But yeah, otherwise it works 🎉 :

[network] Connected to 12D3KooWFXuPMbHyaT7XHp4e8swc4VegV7Rz727JkyizJogrPhjy
[network] Connection(12D3KooWFXuPMbHyaT7XHp4e8swc4VegV7Rz727JkyizJogrPhjy) => ChainConnected(0, 1842118, 0xe225…ca8e)
[network] Connection(12D3KooWFXuPMbHyaT7XHp4e8swc4VegV7Rz727JkyizJogrPhjy) <= GrandpaWarpSyncRequest([195, 36, 22, 206, 14, 110, 15, 69, 189, 19, 191, 30, 63, 14, 240, 152, 223, 9, 55, 202, 183, 56, 218, 253, 255, 33, 97, 205, 11, 33, 14, 116])
[network] Connection(12D3KooWFXuPMbHyaT7XHp4e8swc4VegV7Rz727JkyizJogrPhjy) => GrandpaWarpSyncRequest(Ok(256))
[network] Connection(12D3KooWFXuPMbHyaT7XHp4e8swc4VegV7Rz727JkyizJogrPhjy) <= GrandpaWarpSyncRequest([4, 91, 91, 237, 106, 252, 160, 217, 152, 175, 94, 2, 143, 211, 98, 202, 99, 133, 56, 225, 1, 152, 99, 105, 214, 232, 136, 28, 172, 117, 48, 60])
[network] Connection(12D3KooWFXuPMbHyaT7XHp4e8swc4VegV7Rz727JkyizJogrPhjy) => IdentifyRequest
[network] Connection(12D3KooWFXuPMbHyaT7XHp4e8swc4VegV7Rz727JkyizJogrPhjy) => GrandpaWarpSyncRequest(Ok(256))
[network] Connection(12D3KooWFXuPMbHyaT7XHp4e8swc4VegV7Rz727JkyizJogrPhjy) <= GrandpaWarpSyncRequest([91, 0, 77, 18, 80, 126, 73, 214, 141, 167, 248, 97, 16, 35, 30, 12, 176, 24, 142, 132, 140, 52, 82, 204, 208, 24, 27, 75, 132, 54, 112, 250])
[network] Connection(12D3KooWFXuPMbHyaT7XHp4e8swc4VegV7Rz727JkyizJogrPhjy) => GrandpaWarpSyncRequest(Ok(256))
[network] Connection(12D3KooWFXuPMbHyaT7XHp4e8swc4VegV7Rz727JkyizJogrPhjy) <= GrandpaWarpSyncRequest([5, 193, 163, 37, 26, 193, 92, 56, 114, 45, 176, 220, 133, 200, 110, 24, 4, 203, 110, 118, 208, 255, 175, 2, 62, 47, 111, 53, 254, 222, 103, 50])
[network] Connection(12D3KooWFXuPMbHyaT7XHp4e8swc4VegV7Rz727JkyizJogrPhjy) => GrandpaWarpSyncRequest(Ok(256))
[network] Connection(12D3KooWFXuPMbHyaT7XHp4e8swc4VegV7Rz727JkyizJogrPhjy) <= GrandpaWarpSyncRequest([156, 167, 63, 41, 28, 99, 134, 171, 52, 61, 157, 16, 68, 76, 168, 33, 91, 38, 148, 216, 251, 142, 128, 230, 226, 147, 104, 227, 145, 197, 118, 69])
[network] Connection(12D3KooWFXuPMbHyaT7XHp4e8swc4VegV7Rz727JkyizJogrPhjy) => GrandpaWarpSyncRequest(Ok(256))
[network] Connection(12D3KooWFXuPMbHyaT7XHp4e8swc4VegV7Rz727JkyizJogrPhjy) <= GrandpaWarpSyncRequest([9, 40, 179, 181, 85, 120, 138, 216, 33, 249, 100, 79, 29, 37, 214, 148, 61, 16, 164, 152, 143, 88, 178, 162, 169, 186, 225, 53, 131, 130, 204, 194])
[network] Connection(12D3KooWFXuPMbHyaT7XHp4e8swc4VegV7Rz727JkyizJogrPhjy) => GrandpaWarpSyncRequest(Ok(256))
[network] Connection(12D3KooWFXuPMbHyaT7XHp4e8swc4VegV7Rz727JkyizJogrPhjy) <= GrandpaWarpSyncRequest([82, 96, 203, 219, 111, 119, 36, 126, 125, 68, 77, 150, 154, 226, 148, 237, 35, 216, 158, 165, 82, 34, 245, 52, 14, 246, 151, 227, 194, 117, 78, 40])
[network] Connection(12D3KooWFXuPMbHyaT7XHp4e8swc4VegV7Rz727JkyizJogrPhjy) => GrandpaWarpSyncRequest(Ok(18))
[network] Connection(12D3KooWFXuPMbHyaT7XHp4e8swc4VegV7Rz727JkyizJogrPhjy) <= GrandpaWarpSyncRequest([228, 223, 218, 140, 21, 223, 34, 16, 92, 16, 15, 5, 107, 166, 129, 133, 252, 238, 5, 164, 153, 60, 204, 16, 142, 235, 209, 93, 202, 41, 117, 4])
[network] Connection(12D3KooWFXuPMbHyaT7XHp4e8swc4VegV7Rz727JkyizJogrPhjy) => GrandpaWarpSyncRequest(Ok(1))
[smoldot_js::sync_service] Warp syncing up to block number 1916835, made 8 total warp sync requests.

@andresilva
Copy link
Contributor Author

I added a test for generating / verifying a warp sync proof and added docs. I think this should be good to go now.

@tomaka
Copy link
Contributor

tomaka commented Feb 24, 2021

One thing we were considering adding is a bool in the network message indicating whether the proof is full (and not just truncated to fit in the limit).

This can be done in a further PR, I guess?

Copy link
Contributor

@octol octol left a comment

Choose a reason for hiding this comment

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

Looking good!

@expenses
Copy link
Contributor

@tomaka
Copy link
Contributor

tomaka commented Feb 25, 2021

Let's merge this?

@andresilva
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Feb 25, 2021

Trying merge.

@ghost ghost merged commit 5d72a5a into master Feb 25, 2021
@ghost ghost deleted the andre/fast-warp-sync-proofs branch February 25, 2021 08:44
jam10o-new pushed a commit to jam10o-new/substrate that referenced this pull request Feb 28, 2021
* grandpa: use AuthoritySetChanges to generate warp sync proof

* node: init grandpa warp sync protocol

* grandpa: iterator for AuthoritySetChanges

* grandpa: rewrite warp sync proof generation

* grandpa: remove old code for warp sync generation

* grandpa: fix indentation

* grandpa: fix off by one

* grandpa: use binary search to find start idx when generating warp sync proof

* grandpa: add method to verify warp sync proofs

* grandpa: remove unnecessary code to skip authority set changes

* grandpa: add test for warp sync proof generation and verification

* grandpa: add missing docs

* grandpa: remove trailing comma
thadouk added a commit to Aventus-Network-Services/substrate that referenced this pull request Oct 21, 2021
Part of the upgrade to substrate 3.0
Updating some of the cargo dependencies that the original
packages in substrate 3.0 release are no longer available.

Unbreak browser test CI (paritytech#8148)

Update to libp2p-0.35.1 (paritytech#8141)

Update to libp2p-0.36 (paritytech#8420)

* Update to libp2p-0.36

* Some more Cargo.lock updates.

Update to libp2p-0.35.1 (paritytech#8141)

This commit can be dropped in future upgrades
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants