-
Notifications
You must be signed in to change notification settings - Fork 33
[Core] Consolidate all node configurations into a new configs package - (Issue #396) #403
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.
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
Changes from 30 commits
80eb15b
35b128a
df5c92e
97803dd
d439459
807086f
774e9fd
c88f455
d2f3ba7
ab115f6
80a7ba2
6187b29
fe88719
375fcac
2ea6b95
f599882
43c560f
f60517b
a689e90
b4a5fd9
535130d
00de795
0165bd4
2248e74
fa09298
f71d831
910b20e
4da6cbb
75fa712
31a4013
0b56298
f52182f
8e02599
04c75d4
b54cc52
052c491
01a1fb2
b6d86fd
27db554
20f862d
7396e81
f37f02a
b50bd6a
2b04333
a49a8cd
cf55682
285932f
4e2cc72
829e2ff
a89f218
9dfafb9
7cb47dc
a09c234
7b576df
0a9f151
eb685fb
ceb768e
82e463c
555bdc2
b8b6674
c7d1326
469635a
d9da996
5bef1b1
8ceecb3
8561177
5df685c
f5694ab
196845e
d16a455
7f64ccb
ece9cf8
8305e6e
048ebbc
0db801d
5bacd1b
8a9c0ae
505a104
3cfd657
2c420c7
0d62c5b
10e04cc
e9cc6e8
1e6ceab
623999f
e9ebc6e
4e24c6a
b26663f
be6ba7d
df13c44
c47ddfa
31c79db
00dc7f4
ea999af
9edd1e4
2d58822
834b885
f6100ad
c91a7ad
a4fa535
8d02ad5
ff57847
4bddf5f
0532b07
c86f8c5
69cb8eb
00ae143
5b237f2
8d9cb1a
d7a521e
b8d1946
fb42810
6ff3c3a
414276a
d82b018
6dea794
52b500b
ac0e99d
3524dc9
636638f
2f73d5e
26ef163
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| # Changelog | ||
|
|
||
| All notable changes to this module will be documented in this file. | ||
|
|
||
| The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), | ||
| and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). | ||
|
|
||
| ## [Unreleased] | ||
|
|
||
| ## [0.0.0.1] - 2022-12-28 | ||
|
|
||
| - Removed `BaseConfig` from `configs` | ||
| - Centralized `PersistenceGenesisState` and `ConsensusGenesisState` into `GenesisState` | ||
|
|
||
| ## [0.0.0.0] - 2022-12-22 | ||
|
|
||
| - Introduced this `CHANGELOG.md` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| # Changelog | ||
|
|
||
| All notable changes to this module will be documented in this file. | ||
|
|
||
| The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), | ||
| and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). | ||
|
|
||
| ## [Unreleased] | ||
|
|
||
| ## [0.0.0.1] - 2022-12-28 | ||
|
|
||
| - Refactored configs into `configs` package | ||
|
|
||
| ## [0.0.0.0] - 2022-12-28 | ||
|
|
||
| - Introduced this `CHANGELOG.md` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |
|
|
||
| ## [Unreleased] | ||
|
|
||
| ## [0.0.0.14] - 2022-12-28 | ||
|
|
||
| - `ActorsToAddrBook` now skips actors that are not validators since they don't have a serviceUrl generic parameter | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you explain this? My guess is that this may have caused to some sort of issues/bug and potentially, the solution is to change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently, we use the serviceUrl to connect to peers and to send messages to them, I believe that the https://docs.libp2p.io/concepts/fundamentals/addressing/ Eg: but that's going to be dynamic and transient and I don't see how it would be part of the state. That said, I am not sure we should change the genesis state at all to handle addressing. Also, what is this concept of generic parameter? Where can I learn more about it?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regarding libp2p - makes sense when we get there. Regarding the val filtering - see the comment I left in
This is just a poor design decision that was the result of a compromise during the shared module refactor. We (currently) have 4 protocol actor types:
There are things they all share (e.g. an address) and there are things that are specific:
The two potential future paths we take are:
See #313 for the origin document.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the wholesome explanation and context🙏
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🙏 |
||
|
|
||
| ## [0.0.0.13] - 2022-12-21 | ||
|
|
||
| - Updated to use the new centralized config and genesis handling | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,6 +74,10 @@ func (pabp *persistenceAddrBookProvider) GetStakedAddrBookAtHeight(height uint64 | |
| func (pabp *persistenceAddrBookProvider) ActorsToAddrBook(actors map[string]coreTypes.Actor) (typesP2P.AddrBook, error) { | ||
| book := make(typesP2P.AddrBook, 0) | ||
| for _, v := range actors { | ||
| // only add validator actors since they are the only ones having a service url in their generic param at the moment | ||
| if v.ActorType != coreTypes.ActorType_ACTOR_TYPE_VAL { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TL;DR: The way I see it, NO... Like you said, it looks like these Nodes are listening on the I guess this is because we don't have -yet- a full-fledged P2P module that owns the addressbook and can be relied upon for routing and messaging (that will come with peer discovery and churn). We won't address messages via In If we consider other actors I can only imagine that since we have overlaps (eg: I wish Brian was here to pick his brain on this :) the ways I see it, structured gossip happens under the hood in the P2P module, when you say "broadcast to all validators" you don't know/care if the message is also handled and retransmitted by a Fisherman in order to reach a "nearby" Validator. That's implementation detail of the P2P algorithm. I would say that for now we have to filter. Happy to discuss further even offline. Just let me know how to proceed here.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Solid explanation. I'm convinced. Excited to see what our next steps are going to be here :)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added #426 PTAL 🙏 |
||
| continue | ||
| } | ||
| networkPeer, err := pabp.ActorToNetworkPeer(v) | ||
| if err != nil { | ||
| log.Println("[WARN] Error connecting to validator: ", err) | ||
|
|
||
This file was deleted.



Uh oh!
There was an error while loading. Please reload this page.