node: Add follower node for sync mode#5009
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5009 +/- ##
==========================================
- Coverage 53.62% 53.53% -0.10%
==========================================
Files 432 433 +1
Lines 54057 54277 +220
==========================================
+ Hits 28990 29055 +65
- Misses 22821 22969 +148
- Partials 2246 2253 +7
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| // NodeSyncMode launches the node in "sync" or "data" mode. This turns off agreement service, | ||
| // and APIs related to broadcasting transactions, and enables APIs which retrieve detailed information | ||
| // from ledger caches. | ||
| NodeSyncMode bool `version[26]:"false"` |
There was a problem hiding this comment.
yes, we have PRs open like #5018 collecting v27 changes for the next release currently, v26 already went out in the last one, but if this doesn't make v27 it will end up in v28
There was a problem hiding this comment.
The code duplication is a little disappointing. I know the earlier PRs had some more advanced changes to avoid it, but maybe there is some middle ground?
The changes here are quite straightforward, great complexity improvement over the previous work!
|
Have you tested running a node that just does catchup and has no agreement service? Once you get to the latest block I think you will run into an issue where you will only be checking for a new block every 17 seconds (the value of deadlineTimeout in the code linked below) because catchup is built to assume new blocks are generally always going to come from agreement. However with your sync mode and explicit start/stop stuff maybe you won't run into this? |
winder
left a comment
winder
left a comment
There was a problem hiding this comment.
LGTM, just have some small suggestions
| // NodeFollowerMode launches the node in "follower" mode. This turns off the agreement service, | ||
| // and APIs related to broadcasting transactions, and enables APIs which can retrieve detailed information | ||
| // from ledger caches and can control the ledger round. | ||
| NodeFollowerMode bool `version[27]:"false"` |
There was a problem hiding this comment.
Nit: EnableFollowMode would fit a little better with other settings.
There was a problem hiding this comment.
I kind of don't want it to be lumped in with all of the other "enable" settings just because it's also disabling certain functionality. But the other "Mode" settings are integers that serve as sort of enums whereas this is a boolean so I guess it does make sense to go in the "Enable" category just by virtue of its possible values.
| sRound := v2.Node.GetSyncRound() | ||
| if sRound > 0 && uint64(cpRound) > sRound { | ||
| err = fmt.Errorf("catchpoint round %v greater than sync round %v", cpRound, sRound) | ||
| return badRequest(ctx, err, fmt.Sprintf(errFailedToStartCatchup, err), v2.Log) | ||
| } |
There was a problem hiding this comment.
What do you think about moving this check to v2.Node.StartCatchup or SetCatchpointCatchupMode?
Co-authored-by: cce <51567+cce@users.noreply.github.com>
Co-authored-by: cce <51567+cce@users.noreply.github.com>
algorandskiy
left a comment
algorandskiy
left a comment
There was a problem hiding this comment.
LGTM. Although there is some code duplication but generatlization would require quite a lot code and not sure if it's worth the effort.
Please fix reviewdog remarks and consider adding an e2e test checking the follower can successfully start and serve requests.
| rootDir string | ||
| genesisID string | ||
| genesisHash crypto.Digest | ||
| devMode bool // is this node operates in a developer mode ? ( benign agreement, broadcasting transaction generates a new block ) |
There was a problem hiding this comment.
Thinking about it now, a follower node won't really work with dev mode, right, because it has no transaction pool to assemble its own blocks? Maybe just error if it is configured?
| node.devMode = genesis.DevMode | ||
|
|
||
| if node.devMode { | ||
| cfg.DisableNetworking = true |
There was a problem hiding this comment.
without a transaction pool, and with networking disabled, enabling both follow mode and dev mode at the same time makes this node basically unusable right?
|
|
||
| // BroadcastSignedTxGroup errors in follower mode | ||
| func (node *AlgorandFollowerNode) BroadcastSignedTxGroup(_ []transactions.SignedTxn) (err error) { | ||
| return fmt.Errorf("cannot broadcast txns in sync mode") |
There was a problem hiding this comment.
Not for now, but it would be possible to broadcast signed tx groups using gossip and skip the tx pool, if you didn't want local validation/feedback on your txn
|
@algorandskiy Added e2e test--let me know how it looks. |
4 participants
Adds
AlgorandFollowerNodewhich allows users to control the sync round on the catchup service and disables the agreement service.Also adds the ledger delta APIs to the router when running and data node. This is all put behind a config flag, and a few new interfaces were required to get the APIs to work with both the data node and the full node.