-
Notifications
You must be signed in to change notification settings - Fork 21.5k
Whisper cleanup, part 2 #718
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
Conversation
|
As suggested previously, please don't use rw1, rw2 := p2p.MsgPipe()
p1, p2 := p2p.NewPeer(...), p2p.NewPeer(...)
runWhisper(rw1, p2)
runWhisper(rw2, p1)For testing a single protocol instance, you can use rw1, rw2 := p2p.MsgPipe()
p := p2p.NewPeer(...)
runWhisper(rw1, p)
if err := p2p.Send(rw2, msgcode, ...); err != nil {
t.Error(err)
}
if err := p2p.ExpectMsg(rw2, respcode, ...); err != nil {
t.Error(err)
} |
|
K, will fix :) |
|
Updated the comment above to mention |
|
Please be aware that |
|
Hmmm, but in my integration tests, I was actually testing that the entire thing works as intended. They are not really wire protocol tests. p2p.Send and p2p.Expect would be useful as a lower level test, to verify the packets themselves. Those are the things I wanted to attend to after cleaning up the code. Anyway, I am probably still missing a few things so I'll start digging into these constructs and get back with a proper reply after I know what I'm talking about :D |
whisper/whisper.go
Outdated
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.
Style nit: I'd prefer DefaultTTL, DefaultPoW.
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.
Fixed in f6efdd8
|
@fjl All traces of p2p.Server dumped, and switched over to manual wiring/mocking + added a few lower level protocol tests for whisper.peer-s directly. Whole tests significantly faster now, at around 3 secs for the full suite (cannot really reduce it beyond this, since whisper is based on cyclic messaging, so I do have to wait for the cycles to run). Hope it's better this time :) |
whisper/peer_test.go
Outdated
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.
This comment is not necessary.
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 guess I get carried away sometimes :)), will fix.
|
As general feedback, this looks very good. I haven't looked at some of the changes to the core dispatch yet. One of the general issues with Whisper is that it has not been proven whether the protocol actually works on a big network and what kind of topology it needs to work well. It might be an interesting exercise to test with bigger clusters and several topologies, now that we have the infrastructure for it. But that's for another time. |
|
My current issue with Whisper is that there isn't any (at least I haven't found) easy interface to live-test the thing. I can simulate some connections in the unit tests, or maybe even do some larger simulations by firing up some p2p.Servers, but it would be really nice if we could surface the whisper API into the console in some meaningful way. I saw that there's a javascript implementation, but I got all kind of weird error messages with even a simple function call, and I've no clue yet whether I'm doing something wrong, the JavaScript implementation is buggy at the moment, or the interface between the two somewhere. Do you by any chance have some code base that was based on Whisper and that could be run manually from a CLI without all kinds of fancy QML/Js/etc dependencies? |
|
Btw, I haven't really touched the core logic in whisper. I was mostly just polishing things up, and documenting/testing as I went by. During that I've found a few discrepancies with the spec, a few bugs and some missing but easily addressable things that I've fixed, but no major update yet. I wanted to have a solid base that's also well-ish tested before I start messing too deep with the code. Now I have to figure out how to wire together standalone whisper processes so that I can test that it indeed works in a distributed environment too, as well as connect it to the C++ implementation to see if they actually speak the same protocol or not :) |
The advantage of your current approach (using
No, but you can try the 'Whisper Chat' DApp in Mist. It's in the sidebar. |
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.
This is a cool approach 👍
|
@fjl PTAL |
* crypto/kz4844: pass blobs by ref (ethereum#29050) This change makes use of the following underlying changes to the kzg-libraries in order to avoid passing large things on the stack: - c-kzg: ethereum/c-kzg-4844#393 and - go-kzg: https://github.com/crate-crypto/go-kzg-4844/pull/63 * update version patch * run go mod tidy * use blob reference --------- Co-authored-by: Martin HS <[email protected]> Co-authored-by: Mason Liang <[email protected]>
…hereum#718) This pull request addresses the corrupted path database with log indicating: `history head truncation out of range, tail: 122557, head: 212208, target: 212557` This is a rare edge case where the in-memory layers, including the write buffer in the disk layer, are fully persisted (e.g., written to file), but the state history freezer is not properly closed (e.g., Geth is terminated after journaling but before freezer.Close). In this situation, the recent state history writes will be truncated on the next startup, while the in-memory layers resolve correctly. As a result, the state history falls behind the disk layer (including the write buffer). In this pull request, the state history freezer is always synced before journal, ensuring the state history writes are always persisted before the others. Edit: It's confirmed that devops team has 10s container termination setting. It explains why Geth didn't finish the entire termination without state history being closed. https://github.com/ethpandaops/fusaka-devnets/pull/63/files Co-authored-by: rjl493456442 <[email protected]>
Documentation and refinements for mostly the whole whisper codebase. Additionally: