-
Notifications
You must be signed in to change notification settings - Fork 38
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
Add a IsIncomingP2PEnabled Flag #1491
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good in principle.
A couple of questions.
for _, address := range currentAddresses { | ||
wg.Add(1) | ||
go p.sendBytesWithRetry(&wg, address, msgEncoded) //nolint: errcheck | ||
closureAddr := address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was the purpose of the WaitGroup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it was to issue the payload to all nodes in parallel and wait until the whole broadcast was finished.
With a 2 min retry and one node with p2p off, this had a weird effect on the sequencer which would not update the host db in proper time.
When running the sim with this code, all nodes (host db) have height ~80 but the sequencer (host db) stays at high ~60
go/host/p2p/no_inbound_p2p.go
Outdated
gethlog "github.com/ethereum/go-ethereum/log" | ||
) | ||
|
||
type NoInboundP2P struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how much of this is duplicated compared to the normal implementation?
If a lot, then it would make sense to initialise that with a flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I like using a flag, did that in the p2p in-memory; was keeping them separate to make sure I wasn't introducing any bugs in the existing p2p.
I'll push all under a flag tomorrow 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Why this change is needed
https://github.com/obscuronet/obscuro-internal/issues/2103
To test the system with p2p down.
What changes were made as part of this PR
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks