[Merged by Bors] - Execute tortoise beacon round only if node is synced at the start of the epoch #2715

dshulyak · 2021-08-24T13:17:50Z

Motivation

Tortoise beacon round should be skipped if node is not synced at the start of it.

Tortoise beacon checks that the miner is eligible to send any message during the round (proposal, initial vote, following) using miner weights from the previous epoch. If node doesn't have ATXs at the start of the epoch it is almost guaranteed to produce invalid beacon.

probably related #2699 . will close after verifying

Changes

skip tortoise beacon round if we are not synced at the start of it

Test Plan

it addresses race condition that only appears when new node is added to the network. if this works latenodes test must be stabilized even futher.

TODO

Explain motivation or link existing issue(s)
Test changes and document test plan
Update documentation as needed

DevOps Notes

This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
This PR does not affect public APIs
This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
This PR does not make changes to log messages (which monitoring infrastructure may rely on)

…the epoch

dshulyak · 2021-08-24T13:45:59Z

bors try

bors · 2021-08-24T14:02:13Z

try

Build failed:

ci-stage2

dshulyak · 2021-08-24T14:06:15Z

bors try

bors · 2021-08-24T14:56:52Z

try

Build failed:

ci-stage2

cmd/node/node.go

dshulyak · 2021-08-24T16:39:43Z

bors try

bors · 2021-08-24T17:30:26Z

try

Build failed:

ci-stage2

countvonzero · 2021-08-24T22:13:12Z

tortoisebeacon/tortoise_beacon.go

@@ -156,13 +162,24 @@ type TortoiseBeacon struct {
 	proposalChans   map[types.EpochID]chan *proposalMessageWithReceiptData
 }

+// SetSyncState updates sync state provider. Must be executed only once.
+func (tb *TortoiseBeacon) SetSyncState(sync SyncState) {


rename SetSyncStateProvider? or it sounds like you are changing the state

countvonzero · 2021-08-24T22:15:37Z

tortoisebeacon/tortoise_beacon.go

@@ -359,6 +376,10 @@ func (tb *TortoiseBeacon) handleEpoch(ctx context.Context, epoch types.EpochID)

 		return
 	}
+	if !tb.sync.IsSynced(ctx) {


or you can register a channel to be notified when the node turned synced. see RegisterChForSynced.
tho this does not notify you again if the node goes out of sync, i think it serves your purpose here.

that would be better. but if node goes out of sync again i will need to re-register. and for that, i still need a pointer

countvonzero · 2021-08-24T22:17:44Z

cmd/node/node.go

@@ -661,7 +661,8 @@ func (app *App) initServices(ctx context.Context,
 		AlwaysListen:    app.Config.AlwaysListen,
 	}
 	syncer := syncer.NewSyncer(ctx, syncerConf, clock, msh, layerFetch, app.addLogger(SyncLogger, lg))
-
+	// TODO(dshulyak) this needs to be improved, but dependency graph is a bit complicated


try RegisterChForSynced in syncer.go?

dshulyak · 2021-08-25T06:01:28Z

bors try

bors · 2021-08-25T06:51:19Z

try

Build failed:

ci-stage2

dshulyak · 2021-08-25T06:56:30Z

Anyway this change makes latenodes not less flaky but more, because it misses tortoise beacon in sync consistently now. It can't be merged until we solve that issue.

antonlerner · 2021-08-25T15:12:58Z

Anyway this change makes latenodes not less flaky but more, because it misses tortoise beacon in sync consistently now. It can't be merged until we solve that issue.

I can propose that for the meantime TB validation will not fail block validation, @nkryuchkov has a new syncing method he's working on that should solve this issue so perhaps it's best to patch it now and wait a couple of days to get full functionality of both TB and TB sync

dshulyak · 2021-08-26T06:30:44Z

I can propose that for the meantime TB validation will not fail block validation, @nkryuchkov has a new syncing method he's working on that should solve this issue so perhaps it's best to patch it now and wait a couple of days to get full functionality of both TB and TB sync

sounds good to me. @nkryuchkov do you see any issues with the proposal?

This reverts commit a4ac09a.

nkryuchkov · 2021-08-26T08:35:25Z

@dshulyak no, I don't see any. I did that yesterday in https://github.com/nkryuchkov/go-spacemesh/tree/execute-beacon-only-synced-disable-validation, it increased the values in the test output closer to expected ones, however, there're some still issues in hare (namespace name: pw37j). I tried to disable and mock the tortoise beacon, the test passed, so the issue is in it. I'm fixing hare now

…tion

nkryuchkov · 2021-08-26T09:39:57Z

bors merge

…the epoch (#2715) ## Motivation Tortoise beacon round should be skipped if node is not synced at the start of it. Tortoise beacon checks that the miner is eligible to send any message during the round (proposal, initial vote, following) using miner weights from the previous epoch. If node doesn't have ATXs at the start of the epoch it is almost guaranteed to produce invalid beacon. probably related #2699 . will close after verifying ## Changes - skip tortoise beacon round if we are not synced at the start of it ## Test Plan it addresses race condition that only appears when new node is added to the network. if this works latenodes test must be stabilized even futher. ## TODO - [x] Explain motivation or link existing issue(s) - [ ] Test changes and document test plan - [ ] Update documentation as needed ## DevOps Notes  - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on) Co-authored-by: Nikita Kryuchkov <[email protected]>

bors · 2021-08-26T09:49:19Z

Build failed:

ci-stage2

nkryuchkov · 2021-08-26T09:58:11Z

bors merge

…the epoch (#2715) ## Motivation Tortoise beacon round should be skipped if node is not synced at the start of it. Tortoise beacon checks that the miner is eligible to send any message during the round (proposal, initial vote, following) using miner weights from the previous epoch. If node doesn't have ATXs at the start of the epoch it is almost guaranteed to produce invalid beacon. probably related #2699 . will close after verifying ## Changes - skip tortoise beacon round if we are not synced at the start of it ## Test Plan it addresses race condition that only appears when new node is added to the network. if this works latenodes test must be stabilized even futher. ## TODO - [x] Explain motivation or link existing issue(s) - [ ] Test changes and document test plan - [ ] Update documentation as needed ## DevOps Notes  - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on) Co-authored-by: Nikita Kryuchkov <[email protected]>

nkryuchkov · 2021-08-26T10:05:11Z

bors merge-

bors · 2021-08-26T10:05:12Z

Canceled.

nkryuchkov · 2021-08-26T10:06:54Z

bors merge

…the epoch (#2715) ## Motivation Tortoise beacon round should be skipped if node is not synced at the start of it. Tortoise beacon checks that the miner is eligible to send any message during the round (proposal, initial vote, following) using miner weights from the previous epoch. If node doesn't have ATXs at the start of the epoch it is almost guaranteed to produce invalid beacon. probably related #2699 . will close after verifying ## Changes - skip tortoise beacon round if we are not synced at the start of it ## Test Plan it addresses race condition that only appears when new node is added to the network. if this works latenodes test must be stabilized even futher. ## TODO - [x] Explain motivation or link existing issue(s) - [ ] Test changes and document test plan - [ ] Update documentation as needed ## DevOps Notes  - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on) Co-authored-by: Nikita Kryuchkov <[email protected]>

bors · 2021-08-26T10:15:10Z

Build failed:

ci-stage2

nkryuchkov · 2021-08-26T10:49:47Z

bors merge

…the epoch (#2715) ## Motivation Tortoise beacon round should be skipped if node is not synced at the start of it. Tortoise beacon checks that the miner is eligible to send any message during the round (proposal, initial vote, following) using miner weights from the previous epoch. If node doesn't have ATXs at the start of the epoch it is almost guaranteed to produce invalid beacon. probably related #2699 . will close after verifying ## Changes - skip tortoise beacon round if we are not synced at the start of it ## Test Plan it addresses race condition that only appears when new node is added to the network. if this works latenodes test must be stabilized even futher. ## TODO - [x] Explain motivation or link existing issue(s) - [ ] Test changes and document test plan - [ ] Update documentation as needed ## DevOps Notes  - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on) Co-authored-by: Nikita Kryuchkov <[email protected]>

bors · 2021-08-26T11:40:37Z

Build failed:

ci-stage2

nkryuchkov · 2021-08-26T11:42:39Z

bors merge

…the epoch (#2715) ## Motivation Tortoise beacon round should be skipped if node is not synced at the start of it. Tortoise beacon checks that the miner is eligible to send any message during the round (proposal, initial vote, following) using miner weights from the previous epoch. If node doesn't have ATXs at the start of the epoch it is almost guaranteed to produce invalid beacon. probably related #2699 . will close after verifying ## Changes - skip tortoise beacon round if we are not synced at the start of it ## Test Plan it addresses race condition that only appears when new node is added to the network. if this works latenodes test must be stabilized even futher. ## TODO - [x] Explain motivation or link existing issue(s) - [ ] Test changes and document test plan - [ ] Update documentation as needed ## DevOps Notes  - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on) Co-authored-by: Nikita Kryuchkov <[email protected]>

bors · 2021-08-26T12:31:40Z

Pull request successfully merged into develop.

Build succeeded:

ci-stage2

Execute tortoise beacon round only if node is synced at the start of …

5b64908

…the epoch

dshulyak requested review from nkryuchkov, antonlerner and countvonzero August 24, 2021 13:18

dshulyak added 2 commits August 24, 2021 16:23

Accidental change

ea59b2a

Docstring

6f666f2

bors bot added a commit that referenced this pull request Aug 24, 2021

Try #2715:

fc91856

bors bot added a commit that referenced this pull request Aug 24, 2021

Try #2715:

0ddc535

nkryuchkov approved these changes Aug 24, 2021

View reviewed changes

cmd/node/node.go Show resolved Hide resolved

bors bot added a commit that referenced this pull request Aug 24, 2021

Try #2715:

13a4bf6

countvonzero reviewed Aug 24, 2021

View reviewed changes

countvonzero approved these changes Aug 24, 2021

View reviewed changes

bors bot added a commit that referenced this pull request Aug 25, 2021

Try #2715:

f0d20ea

nkryuchkov added 2 commits August 26, 2021 01:10

Skip validating tortoise beacon

b1c83e1

Temporarily mock tortoise beacon

a4ac09a

Revert "Temporarily mock tortoise beacon"

54045fa

This reverts commit a4ac09a.

Temporarily disable tortoise beacon in hare

5fa30ce

Merge branch 'develop' into execute-beacon-only-synced-disable-valida…

fc087d7

…tion

Fix TestOracle_BuildVRFMessage

8ef7c77

Fix linter

dfcb3d0

Temporarily disable TestBeacon_Value

fbeef78

bors bot changed the title ~~Execute tortoise beacon round only if node is synced at the start of the epoch~~ [Merged by Bors] - Execute tortoise beacon round only if node is synced at the start of the epoch Aug 26, 2021

bors bot closed this Aug 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Execute tortoise beacon round only if node is synced at the start of the epoch #2715

[Merged by Bors] - Execute tortoise beacon round only if node is synced at the start of the epoch #2715

dshulyak commented Aug 24, 2021 •

edited

Loading

dshulyak commented Aug 24, 2021

bors bot commented Aug 24, 2021

dshulyak commented Aug 24, 2021

bors bot commented Aug 24, 2021

dshulyak commented Aug 24, 2021

bors bot commented Aug 24, 2021

countvonzero Aug 24, 2021

countvonzero Aug 24, 2021

dshulyak Aug 25, 2021

countvonzero Aug 24, 2021

dshulyak commented Aug 25, 2021

bors bot commented Aug 25, 2021

dshulyak commented Aug 25, 2021

antonlerner commented Aug 25, 2021

dshulyak commented Aug 26, 2021

nkryuchkov commented Aug 26, 2021

nkryuchkov commented Aug 26, 2021

bors bot commented Aug 26, 2021

nkryuchkov commented Aug 26, 2021

nkryuchkov commented Aug 26, 2021

bors bot commented Aug 26, 2021

nkryuchkov commented Aug 26, 2021

bors bot commented Aug 26, 2021

nkryuchkov commented Aug 26, 2021

bors bot commented Aug 26, 2021

nkryuchkov commented Aug 26, 2021

bors bot commented Aug 26, 2021

[Merged by Bors] - Execute tortoise beacon round only if node is synced at the start of the epoch #2715

[Merged by Bors] - Execute tortoise beacon round only if node is synced at the start of the epoch #2715

Conversation

dshulyak commented Aug 24, 2021 • edited Loading

Motivation

Changes

Test Plan

TODO

DevOps Notes

dshulyak commented Aug 24, 2021

bors bot commented Aug 24, 2021

try

dshulyak commented Aug 24, 2021

bors bot commented Aug 24, 2021

try

dshulyak commented Aug 24, 2021

bors bot commented Aug 24, 2021

try

countvonzero Aug 24, 2021

Choose a reason for hiding this comment

countvonzero Aug 24, 2021

Choose a reason for hiding this comment

dshulyak Aug 25, 2021

Choose a reason for hiding this comment

countvonzero Aug 24, 2021

Choose a reason for hiding this comment

dshulyak commented Aug 25, 2021

bors bot commented Aug 25, 2021

try

dshulyak commented Aug 25, 2021

antonlerner commented Aug 25, 2021

dshulyak commented Aug 26, 2021

nkryuchkov commented Aug 26, 2021

nkryuchkov commented Aug 26, 2021

bors bot commented Aug 26, 2021

nkryuchkov commented Aug 26, 2021

nkryuchkov commented Aug 26, 2021

bors bot commented Aug 26, 2021

nkryuchkov commented Aug 26, 2021

bors bot commented Aug 26, 2021

nkryuchkov commented Aug 26, 2021

bors bot commented Aug 26, 2021

nkryuchkov commented Aug 26, 2021

bors bot commented Aug 26, 2021

dshulyak commented Aug 24, 2021 •

edited

Loading