NDRS-43: add initial storage components#4
Conversation
src/app/config.rs
Outdated
There was a problem hiding this comment.
Derive Debug and add docs maybe?
There was a problem hiding this comment.
This is still missing Debug =)
There was a problem hiding this comment.
Yes - there's no need for a Debug impl here - there'd be no info shown other than the struct's type and we'd never need to print that anyway, since this only used while the tracing subscriber is handling a tracing event.
There was a problem hiding this comment.
I eagerly put the Debug trait on anything where it is cheap as a very weak nod to https://rust-lang.github.io/api-guidelines/interoperability.html .
Debug still shows up in panic handling, for example.
2ddcd44 to
eb29dab
Compare
src/app/config.rs
Outdated
There was a problem hiding this comment.
I thought that the decision to go with 2-tier network hasn't been made yet.
There was a problem hiding this comment.
This repo is still an early WIP effectively. For now, Marc and I are partially pair-programming and partially reviewing via Slack/zoom convos while we build out the basic structure of this repo.
There was a problem hiding this comment.
I agree. This isn't final and not meant to decide this, we can easily pull it out and change it, for example. We just want to put an impl in now.
src/components/storage.rs
Outdated
There was a problem hiding this comment.
I've raised my concern about this couple of times (including questions in https://casperlabs.atlassian.net/wiki/spaces/EN/pages/477659259/Node-rs+Consensus+components+design document) but I really would like to first hear how is reactor going to ensure that no events are lost. We cannot afford to lose PutBlock event when another process is waiting for it. It might result in equivocations that will trigger slashing and validator losing all of his money.
There was a problem hiding this comment.
I'm not sure if there are plans to have a run through of the finalised reactor structure once we finish implementing it. There were some design changes made last night during a prolonged zoom meeting, so these need to be implemented first I'd suggest.
There was a problem hiding this comment.
how is reactor going to ensure that no events are lost
The queue for events is unbounded and infallible. Losing an event thus should not be possible except due to memory exhaustion, which should crash the whole program.
There was a problem hiding this comment.
Or some panic that crashes the program, or a bug, or node operator turning it off, or a power outage, or machine crash etc etc.
There was a problem hiding this comment.
Durability for events is not something we aim for currently, as many depend on volatile state (e.g. retrying a TCP connection would not be something that carries over smoothly to a restarted node).
Panics should be avoided by not calling functions that can panic (or checking preconditions first), they are going to leave your program in an undefined state (from the "business logic" perspective) anyway, so all bets are off. In general, components are expected to never crash and always gracefully handle even "invalid" events, which given the state machine nature, is not hard to do.
Power outage or operator turning it off - persistence would be handled by each component in a meaningful way, outside of the event handling. If it it is really important that an event go through at least once, you'll have to employ transactional techniques inside said component (e.g. similar to a WAL). When recovering from a crash, simply re-schedule these events.
There was a problem hiding this comment.
Transactionality is paramount for the correctness of this software and the success of the project. Any failure to provide exactly-once-delivery and transactionality will result in validators losing money. Previous communication model was giving this implicitly out of the box.
There was a problem hiding this comment.
It's the endpoint of a remote peer in the small network. We'll build up the docs as required once the design stabilises.
There was a problem hiding this comment.
Why do you need an Ord, PartialEq etc for it?
There was a problem hiding this comment.
To be able to tell when an endpoint supercedes another. e.g if you have two endpoints received, take the one with the newer timestamp, if that is a toss-up, go by IP. This is required for conflict resolution in cases where multiple conflicting endpoints might be received.
There was a problem hiding this comment.
Sounds like you're reinventing Kademlia.
There was a problem hiding this comment.
We're not quite there yet! :)
The small network is pretty basic, in that it aims to be fully connected - every node connected to all peers in the network. There's no notion of a routing table structured by k-buckets, and no notion of "XOR-distance" between peers which are absolutely fundamental to Kademlia.
There was a problem hiding this comment.
so if we will have 50 thousands validators everyone will see everyone else?
There was a problem hiding this comment.
so if we will have 50 thousands validators everyone will see everyone else
If we have 50 thousand validators, we will have swapped out this networking component with a better one, possibly based on one of the available Kademlia crates.
This networking module is intended to be a concrete version of a small networking component that can be swapped out later, similar to the Pothole consensus.
marc-casperlabs
left a comment
There was a problem hiding this comment.
Are we maybe going a little overboard with the visilibity restrictions? I usually use pub a little more liberally, since it is a lot less noisy to read.
In the TLS module, many things are no longer exposed at all. Do we pull those back in when we need to?
src/app/config.rs
Outdated
There was a problem hiding this comment.
Newline would be nice (sorry).
There was a problem hiding this comment.
You mean before this line?
src/tls.rs
Outdated
There was a problem hiding this comment.
Hmm, why do we need to hide this behind another module?
There was a problem hiding this comment.
It's purely to keep it out of the crate's public API and hence out of the generated docs since the macro unconditionally makes the struct pub.
I'll add a comment to this effect.
There was a problem hiding this comment.
I'll add a comment to this effect.
👍 👍 👍
I'm less concerned with the noise than that the public API truly reflects what we want it to reflect. I think we'll need to change the visibility a fair bit if/when we decide e.g. components should be separate crates.
That's generally my preferred way of working. Keep the public API as trimmed down as possible while keeping it usable. That can be pretty subjective of course - I'm never sure about how many traits to derive for new structs, since it's not always easy to envisage all the use cases. I think this approach also helps with the docs too - avoiding users of a crate/module getting bogged down reading docs/comments of what should be private implementation details. |
b08a61e to
517e258
Compare
|
I'm kinda with you on the keeping public API surface small - hopefully people will remember where we put the code before they go and reimplement it. |
marc-casperlabs
left a comment
There was a problem hiding this comment.
Good enough for me at this point in time. We will decide the precise shade of red for the shed some time in the future.
517e258 to
f2fc5d7
Compare
Impl JsonSchema for PublicKey and Signature
NDRS-807: Separate block body and header in the database
change client complex args to use checksummed hex decoding
…a_peer-selection
Merge dev into backpressure poc
Update default chainspec values
Binary port tests
Transaction validity is critical to the chain consistency. The specification of the routines is defined in https://github.com/FuelLabs/fuel-specs/blob/master/specs/protocol/tx_validity.md Some of these specs may intersect with VM logic; these intersections are outside the scope of this lib and should be implemented directly in the VM. Resolves #3
The traits were moved to `fuel-storage` so the map requirements won't overlap with the infrastructure types for the VM and client.
# This is the 1st commit message: rebasing # The commit message #2 will be skipped: # WIP # The commit message #3 will be skipped: # WIP # The commit message #4 will be skipped: # WIP # The commit message #5 will be skipped: # WIP # The commit message #6 will be skipped: # Removing notion of install upgrade wasms # The commit message #7 will be skipped: # Removing notion of install upgrade wasms # The commit message #8 will be skipped: # reinstantiated install/upgrade # The commit message #9 will be skipped: # mid additional_computation_factor refactor # The commit message casper-network#10 will be skipped: # mid removing TransactionCategory
No description provided.