feat: gossiping the latest valset in P2P layer in case they get pruned #560

rach-id · 2023-10-25T23:34:41Z

Overview

Closes #556

Checklist

New and updated code has appropriate documentation
New and updated code has new and/or updated testing
Required CI checks are passing
Visual proof for any user facing features like CLI or documentation updates
Linked issues closed with keywords

p2p/dht.go

p2p/validators.go

p2p/validators_test.go

types/latest_valset.go

evan-forbes · 2023-10-26T12:05:36Z

orchestrator/broadcaster.go

@@ -3,6 +3,8 @@ package orchestrator
 import (
 	"context"

+	types2 "github.com/celestiaorg/celestia-app/x/qgb/types"


the standard thing is naming this qgbtypes instead of types2

definitely not blocking, just for future reference, celestiatypes is better, but imo we should be specific with our imports to improve readability. for instance, what happens if we need to import "github.com/celestiaorg/celestia-app/x/blob/types" or "github.com/celestiaorg/celestia-core/types" as well? personally I'm not a fan of naming everything types, but the sdk and tendermint don't agree lol

concerning this, what do you think of making it a rule in CI? I looked for existing tools that can do this but found none, so maybe we can do a simple bash script that takes all the imports in the project, filter those we want to have the same name across all repos, then check whether the right name is used or not.
I don't think it's worth it tbh, it would just make it harder to commit code, but if there is a good reason for it, I'm in

yeah I'm 100% down for adding CI to look at import names. For packages with types I think we could define a linter that is generic enough to be useful. Not sure about other package names that frequently require renaming

evan-forbes · 2023-10-26T12:10:12Z

p2p/dht.go

+// GetLatestValset looks for the latest valset in the DHT.
+// The key will be returned by the `GetValsetKey` method.
+// Returns an error if it fails.
+func (q BlobstreamDHT) GetLatestValset(ctx context.Context) (types2.Valset, error) {
+	encoded, err := q.GetValue(ctx, GetLatestValsetKey()) // this is a blocking call, we should probably use timeout and channel
+	if err != nil {
+		return types2.Valset{}, err
+	}
+	valset, err := types.UnmarshalValset(encoded)
+	if err != nil {
+		return types2.Valset{}, err
+	}
+	return valset, nil
+}


iirc, you mentioned that the the main concern with this approach is that we aren't verifying the valdiator set that is gossipped. Would that be possible though (in a futuer PR ofc)? We should be able to query the current validator set, and from that we should be able to measure the diff in power between the one we recieve that this one afaiu.

does that make sense or am I missing something?

yes, we could, but I didn't want to add that check because it will be checked implicitly when sending the update to Blobstream. So, no need to do it explicitly here

do you mind pointing out that check really quickly for my own understanding? thanks!

It will be done here:

https://github.com/celestiaorg/blobstream-contracts/blob/72b11fbc5ebd10c11e518bfd8b3f3a806d343b59/src/Blobstream.sol#L269-L276

in the contract

but is it possible to gossip and invalid value? also, isn't the issue for folks deploying a new contract? would that check still be hit?

yes, an invalid value can be gossiped, but will be rejected by the contract when relaying events.

For the deployment part, I didn't implement the ability to use the P2P valset to deploy because it's super risky and there is no way to trust it.

types/latest_valset_test.go

evan-forbes · 2023-10-26T12:13:35Z

only some minor comments, but otherwise LGTM

Co-authored-by: Evan Forbes <[email protected]>

rach-id · 2023-10-26T13:36:20Z

Actually, I am facing an issue with the serialization of date to json. If you check the CI, you will see the tests failing, however, they're not failing locally. But when I use a different environment, they still fail expecting different time. So, I am thinking of actually omitting the time during serialization since it's not needed to create the hash or calculate the threshold etc.

rach-id · 2023-10-26T20:49:08Z

@evan-forbes I did 755ff25 to fix the issue of serialization via creating a struct that doesn't have the time field which was troublesome.

The time field is only used for pruning in the state machine. However, it's not part of the hash nor the threshold calculations. So, it made sense to remove it instead of dealing with serialization issues.

What do you think?

evan-forbes

I'm still thinking about the extra gossiping check. I think we might need something like it (see that thread), but that shouldn't block this PR

evan-forbes · 2023-10-27T02:47:05Z

the getting rid of the time makes sense.

in the past with serializing time that needed to be kept, I've often had to overwrite marshaling methods to fix that one annoying type.

rach-id · 2023-10-27T09:33:37Z

I've often had to overwrite marshaling methods to fix that one annoying type.

Can you link me to some code that does this? I'm curious

rach-id · 2023-10-27T13:23:21Z

Added the changes discussed async. Will wait until we test this using an archive node to merge.

codecov-commenter · 2023-10-27T13:26:30Z

Codecov Report

Merging #560 (e5a05a5) into main (77134ee) will increase coverage by 0.56%.
Report is 4 commits behind head on main.
The diff coverage is 37.12%.

@@            Coverage Diff             @@
##             main     #560      +/-   ##
==========================================
+ Coverage   34.23%   34.80%   +0.56%     
==========================================
  Files          26       27       +1     
  Lines        2100     2221     +121     
==========================================
+ Hits          719      773      +54     
- Misses       1295     1354      +59     
- Partials       86       94       +8

Files	Coverage Δ
p2p/keys.go	`83.33% <100.00%> (+0.98%)`	⬆️
p2p/validators.go	`86.25% <100.00%> (+4.25%)`	⬆️
orchestrator/broadcaster.go	`82.35% <40.00%> (-17.65%)`	⬇️
evm/evm_client.go	`50.31% <0.00%> (-1.95%)`	⬇️
p2p/querier.go	`72.85% <0.00%> (-2.15%)`	⬇️
rpc/app_querier.go	`0.00% <0.00%> (ø)`
p2p/dht.go	`65.78% <62.50%> (-6.26%)`	⬇️
orchestrator/orchestrator.go	`2.04% <0.00%> (+0.12%)`	⬆️
types/latest_valset.go	`28.94% <28.94%> (ø)`
relayer/relayer.go	`0.00% <0.00%> (ø)`

rach-id · 2023-10-27T21:31:27Z

I will merge this and test it once we have an archive node ready for testing. And if something is broken, we can fix it in a separate PR.

feat: gossiping the latest valset in P2P layer in case they get pruned

9270c89

rach-id added p2p p2p network related orchestrator orchestrator related relayer relayer related labels Oct 25, 2023

rach-id self-assigned this Oct 25, 2023

rach-id requested a review from evan-forbes as a code owner October 25, 2023 23:34

rach-id commented Oct 26, 2023

View reviewed changes

rach-id added 22 commits October 26, 2023 12:17

Update p2p/dht.go

96642c3

Update p2p/dht.go

7f5f74f

Update p2p/dht.go

bc15575

Update p2p/dht.go

479cab4

Update p2p/dht.go

620b0bb

Update p2p/dht.go

7b0f5fb

Update p2p/errors.go

2a0d5e5

Update p2p/keys.go

25fdfe4

Update types/latest_valset.go

697e0c2

Update p2p/validators.go

21e32ab

Update p2p/validators_test.go

16b712f

Update p2p/validators_test.go

a5b2e50

Update p2p/validators_test.go

09e6954

Update p2p/validators.go

e6b61d2

Update p2p/validators.go

d69f14c

Update p2p/validators.go

8124d1c

Update p2p/validators.go

42e1f02

Update p2p/validators.go

e13e23a

Update p2p/validators.go

24469ea

Update p2p/validators.go

cc536e3

Update p2p/validators.go

9b5baf8

fix: nil pointer

5e726f0

evan-forbes reviewed Oct 26, 2023

View reviewed changes

rach-id and others added 2 commits October 26, 2023 15:12

chore: type2 rename

545ccee

Update types/latest_valset_test.go

4df2e5a

Co-authored-by: Evan Forbes <[email protected]>

fix: introduce LatestValset type to fix serialization

755ff25

evan-forbes previously approved these changes Oct 27, 2023

View reviewed changes

rach-id mentioned this pull request Oct 27, 2023

CI import names #563

Open

rach-id added 2 commits October 27, 2023 15:20

feat: authenticate the valset to the Blobstream contract

d4e6c41

feat: add log message for deployer

e5a05a5

rach-id dismissed evan-forbes’s stale review via e5a05a5 October 27, 2023 13:22

evan-forbes approved these changes Oct 27, 2023

View reviewed changes

rach-id merged commit 791453d into celestiaorg:main Oct 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: gossiping the latest valset in P2P layer in case they get pruned #560

feat: gossiping the latest valset in P2P layer in case they get pruned #560

rach-id commented Oct 25, 2023

evan-forbes Oct 26, 2023

rach-id Oct 26, 2023

evan-forbes Oct 26, 2023

rach-id Oct 26, 2023

evan-forbes Oct 27, 2023

rach-id Oct 27, 2023

evan-forbes Oct 26, 2023

rach-id Oct 26, 2023

evan-forbes Oct 26, 2023

rach-id Oct 26, 2023

evan-forbes Oct 27, 2023

rach-id Oct 27, 2023

evan-forbes commented Oct 26, 2023

rach-id commented Oct 26, 2023

rach-id commented Oct 26, 2023

evan-forbes left a comment

evan-forbes commented Oct 27, 2023

rach-id commented Oct 27, 2023

rach-id commented Oct 27, 2023

codecov-commenter commented Oct 27, 2023

rach-id commented Oct 27, 2023

feat: gossiping the latest valset in P2P layer in case they get pruned #560

feat: gossiping the latest valset in P2P layer in case they get pruned #560

Conversation

rach-id commented Oct 25, 2023

Overview

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes commented Oct 26, 2023

rach-id commented Oct 26, 2023

rach-id commented Oct 26, 2023

evan-forbes left a comment

Choose a reason for hiding this comment

evan-forbes commented Oct 27, 2023

rach-id commented Oct 27, 2023

rach-id commented Oct 27, 2023

codecov-commenter commented Oct 27, 2023

Codecov Report

rach-id commented Oct 27, 2023