Adding ChainSync to spec (WIP) #568
Conversation
|
This is a take on everything we've discussed in last 2-3 weeks around:
This still does not have (still WIP):
|
|
This is unevenly described, too much detail in some places, and not enough detail in others. I am seeking feedback on things unclear. I know there's a lot of key details left unsaid, that i need to write out. Your questions will help me prioritize what to write out. |
graphsync notes/Qs@hannahhoward I'm using a bunch of Grapsync functions in this PR that may not exist. They don't need to, but the functionality is probably needed. May be worth discussing what is needed. AFAIU, the ability to send a selector to a single peer and get back the resulting graph is there. That should be enough for the basics. The other features i'd like to be able to use (but may not be there, and up to us to decide whether to add/when). Marked
|
Qs for @whyrusleeping
Qs for @ZenGround0
Qs for @anorth and @ZenGround0
Qs for @sternhenri and @nicola
Qs for @jzimmerman
|
| - Step 3. Graphsync the `StateTree` | ||
| - **Part 4: Catch up to the chain (`CHAIN_CATCHUP`)** | ||
| - Step 1. Maintain a set of `TargetHeads`, and select the `BestTargetHead` from it | ||
| - Step 2. Synchronize to the latest heads observed, validating blocks towards them |
There was a problem hiding this comment.
You want to make sure the head you selected actually links to your last checkpoint. Naively trying to sync forward towards a target that might not line up is hard (and DoSable)
There was a problem hiding this comment.
- Yes agree - and syncing back just from the head has big DOS attack surface too. [0] need to pick common intermediate targets that the peers who gave us the
TargetHeadscan give us (eg ask for the CID of blocks atn/2, n/4, n/8intervals) or use log skip lists in the state tree ...). haven't written the detail -- will do so. - anyway, In the common case, thankfully, people will download and load the chain manually along w/ the software, and wont have to worry about it. (which is why im Ok hitting testnet and even mainnet w/ this risk -- ie download headers all the way back, then validate fwd.)
[0] Downloading just headers for 1yr of a single chain is on the order of many GB. Downloading that only to find out that head is broken over and over -- reminds of old bittorrent DOS attacks of never giving the last pieces, or sending multi-GB bogus pieces that can only be detected after downloading the whole thing
There was a problem hiding this comment.
(there's a very simple + clean solution using an actor to track TrustedCheckpoints and a log skip list back to it, i'll write it up as a candidate for you to eval)
There was a problem hiding this comment.
I proposed quantizing the heights at which nodes exchange block headers for bootstrapping, so that they remain stable as the chain progresses: #478.
For "Synchronize to the latest heads" does synchronize mean fetch? I appreciate precise language here, and "sync" is already used to refer to the whole process of fetch and validate.
There was a problem hiding this comment.
- Quantizing +1 -- let me review and merge
- i'll also spec out this "log skip list" point too. it's surprisingly simple and surprisingly useful. we can decide together whether to use it or not based on impl time benefits/costs
| c.Graphsync.Pull(c.Peers, sel, c.IpldStore) | ||
|
|
||
| // validate block | ||
| c.ValidateBlock(block) |
There was a problem hiding this comment.
This example is confusing. After some interpretation I think Pull is supposed to pull the state from the state root, but none of the parent blocks. go-filecoin's protocol does something like this but instead pulls the parents and the messages from each block (not the state root). Could you clarify what is being pulled?
There was a problem hiding this comment.
this naive example is off-- mean to say SelectAllBlocksAndMessages (but not StateTrees, or other artifacts)
| must exist in the lookback state). | ||
| - **BV2 - Signature Validation**: Verify `b.BlockSig` is correct. | ||
| - **BV3 - Verify ElectionProof**: Verify `b.ElectionProof` is correct. |
There was a problem hiding this comment.
You mention it for miner address, but this requires having ticket and state at various lookback rounds too.
There was a problem hiding this comment.
Yes, it does. (and with the lookback we can do b.ChainWeight validation too. I'll mention both of these.
| - **Motivating observations:** | ||
| - `Block` propagation is one of the most security critical points of the whole protocol. | ||
| - Bandwidth usage during `Block` propagation is the biggest rate limiter for network scalability. |
There was a problem hiding this comment.
To be clear this is using Block as Fullblock and not Header, correct?
There was a problem hiding this comment.
This should be header plus ordered message cids
There was a problem hiding this comment.
Block in this one sentence is used generally (i'll remove the backticks) to describe that "moving blocks is expensive and bounds scalability, let's break it up into parts"
| - Block propagation begins with the `sender` propagating the `block.SignedBlock` - `Gossipsub.Send(b block.SignedBlock)` | ||
| - This is a very light object (~200 bytes), which fits into one network packet. | ||
| - This has the BlockCID, the MinerAddress and the Signature. This enables parties to (a) learn that a block from a particular miner is propagating, (b) validate the `MinerAddress` and `Signature`, and decide whether to invest more resources pulling the `BlockHeader`, or the `Messages`. |
There was a problem hiding this comment.
Interesting to see that BV2 comes before BV1 and BV0 as framed above.
There was a problem hiding this comment.
- it's because at different times ones make more sense than others.
- eg in
CHAIN_CATCHUP, if a node is receiving/fetching hundreds/thousands ofBlockHeaders, validating signatures is way expensive, and can be deferred in favor of other validation. (ie lots ofBlockHeaderscoming in through network pipe, dont want to bound on sig verification, other checks can help us dump stuff on the floor faster (BV0, BV1) - but in
CHAIN_FOLLOW, we're not receiving thousands, we're receiving maybe a dozen or 2 dozen packets in a few seconds. We receive cid w/ Sig and addr first (ideally fits in 1 packet), and can afford to (a) check if we already have the cid (if so done, cheap), or (b) if not, check if sig is correct before fetching header (expensive computation, but checking 1 sig is way faster than checking a ton). In practice likely that which one to do is dependent on miner tradeoffs. we'll recommend something but let miners decide, because one strat or the other may be much more effective depending on their hardware, on their bandwidth limitations, or their propensity to getting DOSed
| - This is a very light object (~200 bytes), which fits into one network packet. | ||
| - This has the BlockCID, the MinerAddress and the Signature. This enables parties to (a) learn that a block from a particular miner is propagating, (b) validate the `MinerAddress` and `Signature`, and decide whether to invest more resources pulling the `BlockHeader`, or the `Messages`. | ||
| - Note that this can propagate to ther rest of the network before the next steps complete. |
There was a problem hiding this comment.
To be concrete, this means that the pubsub validation step is complete after the previous bullet point of validation?
There was a problem hiding this comment.
Yes, correct.
I'm leaning strongly this way, but i'd like to test this in testnet. I want to see behavior w/ a network of a few hundred nodes + EC.E_LEADERS=10 before committing to "propagate this quickly" or "propagate after you get the header/validate more". (many contributing factors to one or the other working better in practice, and i think this is something that we may have to change (should be small change, reorder ops) if this strategy performs poorly).
There was a problem hiding this comment.
This has fairly significant implementation ramifications, so a clear decision one way or the other would be appreciated (you can change it later, but it'll be expensive).
One significant implication: if the BlockBrief is propagated before a node has fetched the header and messages, then the receiver of a BlockBrief has no idea where to fetch that data from: the sender is not particularly likely to have it. This seems to make the fetching described below not work very well.
On the other hand, if a node does assure that it has the header/messages before propagating, then the BlockBriefs will not zip across the network: they'll move at the speed allowed by the round-trips and transfer of headers and messages.
There was a problem hiding this comment.
Yes, understood. #568 (comment)
There was a problem hiding this comment.
addressed (indicated requirement for step 1).
| - if `receiver` **DOES NOT** already have the full block for `b.BlockCID`, then: | ||
| - if `receiver` has _some_ of the messages: | ||
| - `receiver` requests missing `Messages` and `MessageReceipts` from `sender`: |
There was a problem hiding this comment.
Why are we requesting receipts? We already MUST have them from processing the parent by the time we actually validate a block and could instead validate the root of the receipts tree in the header as the first part of step 5 validation. Fetching receipts seems like wasted bandwidth.
There was a problem hiding this comment.
+1 I don't think we should be transmitting receipts ever.
There was a problem hiding this comment.
yep, my mistake -- will fix
| - **Step 2. (receiver) `Pull BlockHeader`**: | ||
| - if `receiver` **DOES NOT** already have the `BlockHeader` for `b.BlockCID`, then: | ||
| - `receiver` requests `BlockHeader` from `sender`: |
There was a problem hiding this comment.
Throughout this section the spec calls for fetching from the sender. Who is sender, the latest gossipsub router or the pubsub originator? In either case this could be overly prescriptive. Particularly in the case of messages most other (non-sender) peers should be able to fulfill the Pull Messages request and the network might operate more efficiently if the protocol gives them the freedom to do so.
Guessing this isn't the case, but if the sender is the original publisher that would be problematic because the whole network would hit that node at once.
There was a problem hiding this comment.
- yes
senderis the latest gossipsub router. - yes, dont ask original publisher -- "hey i mined a block!" -> DDOS 😄
- yes,
receivercould definitely ask any other party. but the best case is to asksenderbecause it leverages pubsub/gossipsub fanout choices, correlates to who has it already (you got an advertisement first fromsender, you want to ask the first party you hear from, and you're not yet sure anybody else has it. if they dont send it, yes ask someone else, a second laterreceivershould've heard from many others also having the block). - basically,
receiveris going to receive N advertisements for each winning block in an epoch, in quick succession. (a) want propagation as fast as reasonable, (b) want to make those advertisements as light as reasonable, (c) want to enablereceiverto choose who to ask it from (usually the first party to advertise it, and that's what spec will recommend), and (d) want to be able to fall back to asking others if that fails (fail = dont get it in 1s or so, that fast)
| - **Step 4. (receiver) `Pull Messages`**: | ||
| - if `receiver` **DOES NOT** already have the full block for `b.BlockCID`, then: | ||
| - if `receiver` has _some_ of the messages: |
There was a problem hiding this comment.
I want to clarify that I believe this some messages condition requires that the receiver ran step 3 and has message hints.
There was a problem hiding this comment.
As far as I can tell, the only way a node can know if it has the full block, or if it has some of the messages, is for step 3 or 4 to have happened. Without step 3, a node must pull all messages (SelectAll(bh.Messages)), even if only to learn that it had them all in the first place. Thus unless something like Step 3 becomes expected, all nodes will pull all messages all the time (despite having most of them in the message pool).
There was a problem hiding this comment.
Yes, the "if receiver has some of the messages:" line assumed Step 3. For completeness, i'll add that MessageHints could be the list of CIDs (which will be smaller)
| - **security conditions to transition out:** | ||
| - `Network` connectivity MUST have reached the security level acceptable for `ChainSync` | ||
| - `BlockPubsub` connectivity MUST have reached the security level acceptable for `ChainSync` |
There was a problem hiding this comment.
Are these security levels specified by the protocol? Seems like it could have network wide implications if everyone chooses lax security here but maybe not so its ok to leave up to user?
There was a problem hiding this comment.
Yes, i want to set strict parameters here. need to do some modeling to get the right numbers, but ballpark is: ~32 peers in the minimum, in both direct conns and gossipsub. ideal would be something larger, like 64/128 peers
|
In terms of what graphsync can do -- I can't seem to check stuff on your comment, but here's my point by point:
You may want to look at where the selector spec landed: It can give you a sense of what selectors do and don't support. |
There was a problem hiding this comment.
I struggled to grok the high-level overview of how the catchup mechanics are expected to work. I didn't see anything that contradicts my expectations coming in, but neither where they soundly ratified.
Is the essential process still:
- bootstrap to determine a target head tipset
- fetch all the headers backwards to the prior validated tipset (or genesis if none)
- fetch the messages and validate state transitions forwards from prior validated state up to this target head
- either repeat, or continually maintain the target, until validated to "close enough" to switch to maintaining sync
| We expect that many nodes will synchronize blocks this way, or at least try it first, but we note | ||
| that such implementations will not be synchronizing securely or compatibly, and that a complete | ||
| implementation must include the full `ChainSync` protocol as described here. |
There was a problem hiding this comment.
Since the above is so trivially, even accidentally attackable, I suggest omitting it from the spec.
There was a problem hiding this comment.
Yes i'll most likely remove it
| - Resource expensive verification MAY be skipped at nodes' own risk | ||
| - **Part 2: Bootstrap to the network (`BOOTSTRAP`)** | ||
| - Step 1. Bootstrap to the network, and acquire a "secure enough" set of peers (more details below) | ||
| - Step 2. Bootstrap to the `BlockPubsub` channels |
There was a problem hiding this comment.
Go-filecoin at present goes to some effort to avoid subscribing to pubsub until after bootstrapping and chain catchup are complete. The target heads are obtained through the Hello protocol, and pubsub ignored until the node has reliably synced to "close enough" to the height of blocks propagated over pubsub. This allows easy rejection of pubsub blocks that are too far away from the expected height and limits the work that must be done to fetch and validate a block that turns out to be invalid (e.g. invalid final state root).
I infer that the subscription here is for peer discovery purposes, favouring gossipsub-based discovery over KademliaDHT (that go-filecoin currently uses). Is that the case?
There was a problem hiding this comment.
We use block pubsub from the get-go to ensure the sync process is always adequately informed. Theres a lot that can be done concurrent to the primary sync process to speed up and/or increase the certainty in the selected chain
There was a problem hiding this comment.
- The subscription is for both peer discovery and to start selecting best heads. the thing is the hello protocol forces polllng frequently and one could end up syncing to the wrong head(s) (reorgs). Listing on pubsub from the start keeps the node informed about potential head changes.
- Kademlia PeerDiscovery is great too -- i mostly listed it as optional in that people don't need to implement the dht if they haven't already, but they do need to implement gossipsub and that might give them enough peer discovery. (if you have the dht, feel very able to use it. it will provide peerRouting for you too, which is a great plus. i dont think we need to require it, (maybe we do,
| - Step 3. Graphsync the `StateTree` | ||
| - **Part 4: Catch up to the chain (`CHAIN_CATCHUP`)** | ||
| - Step 1. Maintain a set of `TargetHeads`, and select the `BestTargetHead` from it | ||
| - Step 2. Synchronize to the latest heads observed, validating blocks towards them |
There was a problem hiding this comment.
I proposed quantizing the heights at which nodes exchange block headers for bootstrapping, so that they remain stable as the chain progresses: #478.
For "Synchronize to the latest heads" does synchronize mean fetch? I appreciate precise language here, and "sync" is already used to refer to the whole process of fetch and validate.
| - **Part 4: Catch up to the chain (`CHAIN_CATCHUP`)** | ||
| - Step 1. Maintain a set of `TargetHeads`, and select the `BestTargetHead` from it | ||
| - Step 2. Synchronize to the latest heads observed, validating blocks towards them | ||
| - Step 3. As validation progresses, `TargetHeads` and `BestTargetHead` will likely change |
There was a problem hiding this comment.
Because of information obtained through validation, or just because time passes and the node should be continually receiving new candidates heads from other nodes?
There was a problem hiding this comment.
Both:
- validation can show some heads are bad heads (pruned out, maybe our
BestTargetHeadis no longer good) - time passes, node is receiving new, better candidate heads, either building on top of the previous one, or reorgs
(will clarify)
| - Step 2. Get the block it points to, and that block's parents | ||
| - Step 3. Graphsync the `StateTree` | ||
| - **Part 4: Catch up to the chain (`CHAIN_CATCHUP`)** | ||
| - Step 1. Maintain a set of `TargetHeads`, and select the `BestTargetHead` from it |
There was a problem hiding this comment.
Please define what TargetHeads are supposed to be. The name implies a fringe, i.e. only tipsets with no children - is that correct?
There was a problem hiding this comment.
- thanks -- will define. supposed to be
Blocks(which point to/define aTipset) - saw other comment, i'll think through whether this would be much better as a
Tipset(instead ofBlock) and i'll update here
| // ToValidateG is the set of unvalidated Blocks fully connected | ||
| // to the validated graph. These are ready to be validated, all the | ||
| // data is local, and are ordered by total ChainWeight. | ||
| ToValidateG PartialGraph |
There was a problem hiding this comment.
I don't think the chain weight can be computed until after validation (execution of all messages to discover what the state, in particular the power table, looks like).
EDIT: ah, the block headers declares its weight, though this isn't validated yet.
There was a problem hiding this comment.
To follow the naming pattern of predicates describing the data, suggest calling this FetchedG.
There was a problem hiding this comment.
- ah, the block headers declares its weight, though this isn't validated yet.
Yeah, you can't trust it, but you can use it. (would be awesome to have a publicly verifiable proof there... would make everything easier...)
FetchedG
SGTM! will fix
| // Obviously bad heads SHOULD be pruned out: | ||
| // - Clearly invalid ChainWeight (lower than BestHead or too high) | ||
| // - Heads that are not advancing in BlockPubsub | ||
| TargetHeads [block.BlockCID] |
There was a problem hiding this comment.
I have assumed all the way through here that "head" is a tipset, not a single block. This and BestTargetHead contradict that. I think we might need a new word to refer to blocks that area at the fringe (have no children), distinct from tipsets that can be composed from those blocks.
There was a problem hiding this comment.
- Yes -- sorry for the ambiguous word (head). maybe i should switch to
TargetBlocksandBestTargetBlock- i can also rename in terms of
Tipset, but thinking/dealing with them as tipsets is a bit trickier (combinatorics...)
- i can also rename in terms of
- I believe it is equivalent to do it in terms of Blocks (because each block uniquely defines a tipset, in the prior epoch, like the StateTree, and merely proposes messages for the next round) -- and it's easier to reason/program -- but this is flexible, can opt for what we prefer most
-
I think we might need a new word to refer to blocks that area at the fringe (have no children), distinct from tipsets that can be composed from those blocks.
- yes, i was using "Head" that way -- maybe
HeadBlockvsHeadTipset? hmmm open to other suggestions
There was a problem hiding this comment.
I'm ok with most of this sync spec being phrased in terms of blocks. It does simplify a few things. But "head" in particular is ambiguous, so qualifying it always would help.
There was a problem hiding this comment.
(for now keeping word Head but clearly noting it is a block. may still switch it before PR lands.
| Heads [block.BlockCID] | ||
|
|
||
| // Tails is the set of leaf/tail blocks | ||
| Tails [block.BlockCID] |
There was a problem hiding this comment.
I'm confused. Does head refer to the newest thing, as in the "head of the chain" being the latest state? The use of "root" is confusing if so, since one might usually identify that with the genesis. Similarly "leaf" also sounds like the expanding fringe.
There was a problem hiding this comment.
yeah this is an annoying nomenclature. this partial graph is a DAG fragment-- including disconnected components. here's a visual example, 4 example PartialGraphs, with Heads and Tails. (note they aren't tipsets)
|
|
||
| // Remove deletes given BlockCID from the PartialGraph, updating | ||
| // Blocks, Roots, and Leaves as needed. | ||
| Remove(c block.BlockCID) |
There was a problem hiding this comment.
Is a graph necessarily connected, or is this just a bag of blocks, with each disconnected subgraph being reported in heads/tails?
There was a problem hiding this comment.
is a graph necessarily connected, or is this jut a bag of blocks, with each disconnected subgraph being reported in heads/tails
the latter -- these are 4 examples of ParialGraphs, the 3rd and 4th case are separate chains.
| // from here, we may consider many tipsets that _include_ Block | ||
| // but we must indeed include t and not consider tipsets that | ||
| // fork from Block.Parents, but do not include Block. | ||
| type Checkpoint BlockCID |
There was a problem hiding this comment.
With tipsets, using a single block here confuses me a bit. Things that would make more transparent sense would be a state CID, or a tipset identifier (list of block CIDs).
There was a problem hiding this comment.
-
The reason a block is OK is that it uniquely identifies a tipset.
-
using tipsets directly would make Checkpoints harder to communicate. we want to make checkpoints a single hash, as short as we can have it. They will be shared in tweets, URLs, emails, printed into newspapers, etc. Compactness, ease of copy-paste, etc matters.
-
we'll make human readable lists of checkpoints, and making "lists of lists" is much more annoying. compare:
> cat block-checkpoints bafy2bzacedkf2yx3l7qmjklhylug64v2wnzw2punqiazcldtdkztq6x4745bm bafy2bzacecqgqed3x4atzznobctanup4cvkutiwdrlw7ujyl56k64yn3da4jm bafy2bzacecmqjutm36dq6kwzwuzstsagplx6rjunikpnnopocxcq5uxgby6rg bafy2bzaceccbbedox2bdf5qw2owdgurf5xrgbd6fthyt3menb6qxkgrdo7tza # vs delimiting a list of lists (space, commas, json arrays...) > cat tipset-checkpoints bafy2bzacedkf2yx3l7qmjklhylug64v2wnzw2punqiazcldtdkztq6x4745bm bafy2bzacecqgqed3x4atzznobctanup4cvkutiwdrlw7ujyl56k64yn3da4jm bafy2bzacecmqjutm36dq6kwzwuzstsagplx6rjunikpnnopocxcq5uxgby6rg bafy2bzaceccbbedox2bdf5qw2owdgurf5xrgbd6fthyt3menb6qxkgrdo7tza, bafy2bzacedkf2yx3l7qmjklhylug64v2wnzw2punqiazcldtdkztq6x4745bm -
When we have EC.E_PARENTS > 5 or = 10, tipsets will get annoyingly large.
-
the big quirk/weirdness with blocks it that it also must be in the chain. (if you relaxed that constraint you could end up in a weird case where a checkpoint isnt in the chain and that's weird/violates assumptions -- eg below).
There was a problem hiding this comment.
So how about a state CID instead of a Block CID?
Note that the genesis state is not constructable as a sequence of messages applied to an empty state (it contains system actors already installed). We actually don't need a genesis block so much as a genesis state. What would break if multiple miners produced blocks in round 1, all sharing the same parent state root?
There was a problem hiding this comment.
So how about a state CID instead of a Block CID?
I'm not seeing how this is better? maybe im missing it?
I understand some of the motivation -- the genesis block seems superfluous, in light that it points back to a parent StateTree anyway. And I see that the algorithms here can be changed to reason also about StateTrees as checkpoints, and to deal with cases of syncing blocks back to a StateTree instead of syncing blocks back to another block (this would add a lot StateTree oriented checks in addition to all the block oriented checks we already need).
this would change the logic of tools and processes/expectations around checkpoints that already exist in broader community w/ other chains -- this can be changed, but we would have to make sure a lot of tools supported looking up StateTree by cid (tools which already would support lookup of blocks by cid but not necessarily a StateTree, eg simple block explorers, wallets, chain exporters, etc). this is different and would need more work, but doable.
Im not seeing lots of benefit for these changes and additional tooling work -- but maybe im missing them? is there more beyond the conceptual re-writing and cleaning up of checkpoints? (namely, that they always point to a "thing" at a particular round of which there is only one valid instance-- ie only one StateTree per round)
I would be OK doing this (changing to use StateTree as the checkpoint) if StateTree also became the "root object" and included the blocks within it (as in ETH2, and some other proposed chains, which treat consensus as a VM-internal process, not VM-surrounding process). Making StateTree the root object would cement its standing as a key, root data structure, that would need to get support in a ton of tools. (but would still have a genesis block, see below.)
Note that the genesis state is not constructable as a sequence of messages applied to an empty state (it contains system actors already installed).
Yes
We actually don't need a genesis block so much as a genesis state.
I believe we do need a genesis block -- we derive randomness from the ticket there. Rather than special casing, it is easier/less complex to ensure a well-formed chain always, including at the beginning
What would break if multiple miners produced blocks in round 1, all sharing the same parent state root?
invariants/tools expecting the chain to end at one block (this reminds of git's empty repo and the edge cases it introduces, instead of starting always with the same empty/first commit).
In any case, the mainnet filecoin chain will need to start with a small chain stub of blocks.
- product reason, we actually have to include some data in different blocks. (dont worry about this at all till Q1 -- we'll produce the relevant chain stub)
- A lot of code expects lookbacks, especially actor code. Rather than introducing a bunch of special case logic for what happens ostensibly once in network history (special case logic which adds complexity and likelihood of problems), it is easiest to assume the chain is always at least X blocks long, and the system lookback parameters are all fine and dont need to be scaled in the beginning of network's history.
There was a problem hiding this comment.
Ok, thanks. You understand my motivations and this proposal is reasonable.
|
Is the hello protocol now irrelevant, to be removed? Using gossipsub as a required peer discovery mechanism is new for GFC, which currently relies on DHT traversal. I'm interested in the motivation for this. |
| func (c *NaiveChainsync) Sync() { | ||
| // for each of the blocks we see in the gossip channel | ||
| for block := range c.BlockPubsub.NewBlocks() { | ||
| c.IpldStore.Add(block) |
There was a problem hiding this comment.
is this assuming that the BlockPubsub validator is checking the high level (stateless) validity of the block?
There was a problem hiding this comment.
Yes -- i still need to think through how much we want/can do at the BlockPubsub level (see the Progressive Block Validation section below)
|
|
||
| go func() { | ||
| // fetch all the content with graphsync | ||
| selector := ipldselector.SelectAll(block.CID) |
There was a problem hiding this comment.
select all here is kinda scarily undefined, does this imply we sync every cid linked by this block, recursively? This would imply syncing the state trees of every single block in the entire chain, something very expensive to do
There was a problem hiding this comment.
- yeah this was a toy example, and yes, bug. I'll define a better selector for this example (fetch the chain + messages, not state trees)
- im not sure how much more state is syncing all the state trees (because of persistent data structure properties, this may not be, say, 100x larger. i may only be 5x larger)
| - _(TODO: move this to blockchain/Block section)_ | ||
| - **BV0 - Syntactic Validation**: Validate data structure packing and ensure correct typing. | ||
| - **BV1 - Light Consensus State Checks**: Validate `b.ChainWeight`, `b.ChainEpoch`, `b.MinerAddress`, are plausible (some ranges of bad values can be detected easily, especially if we have the state of the chain at `b.ChainEpoch - consensus.LookbackParameter`. Eg Weight and Epoch have well defined valid ranges, and `b.MinerAddress` |
There was a problem hiding this comment.
does this or does this not depend on having some amount of chain history linking to this block?
There was a problem hiding this comment.
- I see this one (BV1) as best effort, given the available chain -- this means that a node should be able to estimate valid ranges for
b.ChainEpochbased on theLastTrustedCheckpoint. `b.ChainWeight is harder unless we have some of the relatively recent chain. - This is not meant to have the
StateTreeatb.ChainEpoch - Lookback. (this may be an additional stage, or just part ofBV3)
| - Note that this can propagate to ther rest of the network before the next steps complete. | ||
| - **Step 2. (receiver) `Pull BlockHeader`**: | ||
| - if `receiver` **DOES NOT** already have the `BlockHeader` for `b.BlockCID`, then: |
There was a problem hiding this comment.
So this implies that you want to not send out the block header? I'm confused, that seems like unnecessary round trips.
And if its not the block header, what are the 200 bytes you mention?
There was a problem hiding this comment.
"This has the BlockCID, the MinerAddress and the Signature."
There was a problem hiding this comment.
yes, as @ZenGround0 said:
type SignedBlock (or BlockStamp) struct {
CID BlockCID
MinerAddr addr.Address
Sig block.Signature
}
- yes it's a round trip, BUT when you're getting a ton of these from 30 or 100 peers simultaneously, it's much better to get 30 or 100 packets than 120 or 400 packets, especially when a large fraction of the sends you get will be duped.
There was a problem hiding this comment.
- gossipsub defaults to a fanout of 6, which sounds ok, but im not sure is secure. (i believe bitcoin averages 32).
- with
EC.E_LEADERS=10, we may be getting 20-60 "blocks" (advertisements of new block) through in a short period, making these cost 1 packet instead of 4 is meaningful. - Especially if we bump up
gossipsub.GossipSubDto 20 or 40. (learning about new blocks ASAP, and more securely (bigger fanout, less isolatable) matters) - this is actually roughly what btc and eth1 do. (advertising hashes to the majority, sending headers to a few.
- i believe bitcoin uses
invmessages to advertise knowledge of a block (1 RTT) before sending the header, not sure what distribution of the network does this vs pushing headers always. (bitcoin header is not much bigger...) -- i believe most miners run custom versions here w/ cmpctblock and similar. (btw aww envelopes / multicodecs) -
In order to avoid sending transaction and block messages to nodes that already received them from other nodes, they are not forwarded directly. Instead their availability is announced to the neighbors by sending them an inv message once the transaction or block has been completely verified. The inv message contains a set of transaction hashes and block hashes that have been received by the sender and are now available to be requested.
- i believe bitcoind has (or proposed) settings where, on a per-connection basis, peers can agree to send full headers or even full blocks right away. this SGTM, but the complexity isnt worth it atm. maybe later.
- ETH1:
Blocks are typically re-propagated to all connected peers as soon as basic validity of the announcement has been established (e.g. after the proof-of-work check). Propagation uses the NewBlock and NewBlockHashes messages. The NewBlock message includes the full block and is sent to a small fraction of connected peers (usually the square root of the total number of peers). All other peers are sent a NewBlockHashes message containing just the hash of the new block. Those peers may request the full block later if they fail to receive it from anyone within reasonable time.
There was a problem hiding this comment.
btw afaict, gossipsub does not use the latency info from libp2p conns to prefer sends. it should prefer some (not all) into the fanout selection -- simple addition that will speed up delivery. not all because insecure/incorrect. need randomization. bumping up fanout, w/ very small messages, and adding preferring latency should perform well
| - This is a light-ish object (<4KB). | ||
| - This has many fields that can be validated before pulling the messages. (See **Progressive Block Validation**). | ||
| - **Step 3. (receiver) `Pull MessageHints` (TODO: validate/decide on this)**: |
There was a problem hiding this comment.
This makes propagation of blocks a multi-round trip process, and this call alone will make the bandwidth concerns worse than just sending {header, msgscids}. Really not sure what is being gained here.
There was a problem hiding this comment.
overhead of the calls is not worse than sending all msgcids. MessageHints could be 2-8 bytes per message (depending on msgpool size), so with 5000 messages (reasonable even in a 250KB block, and reasonable given our deal expectations), we're talking a difference of (pls check my math):
- MessageHints: 5000 x {2, 6, 8, 10, 12}B = {10KB, 36KB, 40KB, 50KB, 60KB}
- MessageCIDs: 5000 x 34B = 170KB
- what's the overhead of an additional RPC? 1 more packet, not 100KB
the roundtrip can save significant (heavily duplicated) bandwidth, in a critical period of time. The roundtrip comes at the cost of about 40-300ms of latency -- w/ something like 10-20 peers, most parties will have some peers in <120ms RTT
| - **transitions out:** | ||
| - if a temporary network partition is detected: move to `CHAIN_CATCHUP` | ||
| - if gaps of >2 epochs form between the Validated set and `ChainSync.BestTargetHead`: move to `CHAIN_CATCHUP` |
There was a problem hiding this comment.
2 ? that will happen frequently. Null blocks will happen quite often that cause gaps like this
There was a problem hiding this comment.
- Great catch, null blocks shouldn't count for this because they dont introduce validation delay. sorry, should've written that in.
- making it easy to transition to/from
CHAIN_CATCHUP <-> CHAIN_FOLLOWis the ideal situation here (this can just be a binary switch in a protocol, that just decides to advance finality or not). the important part is halting the advancing of finality till we're really at the fringe. a string of 5 null blocks (which would be very rare with EC.E_LEADERS >5, or =10) signals a probable network partition and warrants pausing finality advancement, validating until we catch up. finality is serious business and annoying to recover from mistakes.
There was a problem hiding this comment.
Answers to questions
Can you look through the ChainSync Summary and see if it makes sense given our conversations about finality and checkpoints?
Generally looks ok. See comments below with concerns.
Can you look through CHAIN_CATCHUP and CHAIN_FOLLOW and lmk how close that is to what you do already? how different is this from what you do already?
It is fairly different but the spec is in the right direction. We don't really differentiate between these states and use a single mechanism that covers both by syncing sections of chains through graphsync and validating them.
Can you look through the Bootstrap state?
This section is strong and helpful
Can you look through the FSM and lmk what here seems infeasible/too hard?
It all looks doable once we converge on details. I think everyone agrees we can and should skip writing the SYNC_CHECKPOINT phase.
Other thoughts
The spec has lots of great detail on how to sync blocks in CHAIN_FOLLOW and has almost nothing for how to sync in CHAIN_CATCHUP. The successive propagation section doesn't help with fetching blocks from the past that don't show up on pubsub. Confusingly there are lots of partial graph datastructures and other hints lying around indicating that there is an expectation of doing something specific to handle CHAIN_CATCHUP. Lots of unanswered questions including:
- Does syncing large sections backwards with graphsync as gofc does now still work or should we be syncing forward somehow?
- Are messages and headers supposed to arrive separately as in CHAIN_FOLLOW or can we keep fetching full blocks during CHAIN_CATCHUP.
- How do we interleave syncing from target heads and updating those heads?
- Do nodes propagate over gossipsub during CHAIN_CATCHUP?
Along the same lines separating target head management into its own concept is a great idea. But currently it is also underspecified.
Throughout the comments there is talk about what we can leave out until mainnet. I would like us to come to a well understood agreement on that once the spec details are agreed upon.
| - `ChainSync` is receiving latest `Blocks` from `BlockPubsub` | ||
| - `ChainSync` starts fetching and validating blocks (see _Block Fetching and Validation_ above). | ||
| - `ChainSync` has unvalidated blocks between `ChainSync.FinalityTipset` and `ChainSync.TargetHeads` |
There was a problem hiding this comment.
Using ChainSync.FinalityTipset is confusing because the spec says nothing about how it is set when entering this state from BOOTSTRAP. I'm guessing in that case its the tipset corresponding to the LastTrustedCheckpoint?
There was a problem hiding this comment.
Yes, correct. if LastTrustedCheckpoint is our latest finalized tipsed, use that as ChainSync.FinalityTipset. will write it up
| - Note that this can propagate to ther rest of the network before the next steps complete. | ||
| - **Step 2. (receiver) `Pull BlockHeader`**: | ||
| - if `receiver` **DOES NOT** already have the `BlockHeader` for `b.BlockCID`, then: |
There was a problem hiding this comment.
"This has the BlockCID, the MinerAddress and the Signature."
| - if `receiver` has _some_ of the messages: | ||
| - `receiver` requests missing `Messages` and `MessageReceipts` from `sender`: | ||
| - `Graphsync.Pull(sender, Select(m3, m10, m50, ...))` |
There was a problem hiding this comment.
This also requires that nodes store both message collections and each individual message on disk to enable propagation.
| This means all blocks _before_ the best heads. | ||
| - Not under a temporary network partition | ||
| - **transitions out:** |
There was a problem hiding this comment.
This spec optimistically ignores the case where our target heads all fork off before the FinalityTipset. As written the spec halts nodes that have been left behind on a fork because (1) these nodes stay in CHAIN_CATCHUP and by the spec aren't supposed to mine and fork the chain and (2) these nodes are not allowed to accept new chains that fork past finality to get out of CHAIN_CATCHUP.
What is the solution here? Do we delegate to the operator to restart the node at this point to make progress? Should some fallback from CHAIN_CATCHUP to bootstrap be specified?
There was a problem hiding this comment.
This spec optimistically ignores the case where our target heads all fork off before the FinalityTipset. As written the spec halts nodes that have been left behind on a fork because (1) these nodes stay in CHAIN_CATCHUP and by the spec aren't supposed to mine and fork the chain and (2) these nodes are not allowed to accept new chains that fork past finality to get out of CHAIN_CATCHUP.
no, that's why it's crucial NOT to advance finality in CHAIN_CATCHUP, see this part: - Chain State and Finality above. (great that you caught this as an optimistic thing and potential problem, but that's why i explicitly separated CHAIN_CATCHUP (validate, but dont advance finality) from CHAIN_FOLLOW (advance finality)
What is the solution here? Do we delegate to the operator to restart the node at this point to make progress? Should some fallback from CHAIN_CATCHUP to bootstrap be specified?
delegating to restart is always an option, but tricky. best to have a protocol that works almost all the time
There was a problem hiding this comment.
Hmm I think I'm talking about a less dangerous but harder to fix problem. The situation I'm flagging is when a node's finalized tipset as set during FOLLOW is already on a fork off of the main chain (due to undetected network partition for example). The observation is that the existing spec of CATCHUP honors the finality condition set in follow, even though this condition was potentially based on bad assumptions.
I think this is not an issue if we trust that the network partition detector doesn't have false negatives.
There was a problem hiding this comment.
The observation is that the existing spec of CATCHUP honors the finality condition set in follow, even though this condition was potentially based on bad assumptions.
ah yes. indeed.
I think this is not an issue if we trust that the network partition detector doesn't have false negatives.
Yes, or rather: unlikely and should be fixed manually.
There was a problem hiding this comment.
Thank you @anorth @ZenGround0 @whyrusleeping for your thoughtful review! 👍 left a lot of responses. i will do another round incorporating all this feedback into the doc, and ping again when i push. hopefully this helped answer some Qs, this sorted out some bugs for me, thanks!
| - Resource expensive verification MAY be skipped at nodes' own risk | ||
| - **Part 2: Bootstrap to the network (`BOOTSTRAP`)** | ||
| - Step 1. Bootstrap to the network, and acquire a "secure enough" set of peers (more details below) | ||
| - Step 2. Bootstrap to the `BlockPubsub` channels |
There was a problem hiding this comment.
- The subscription is for both peer discovery and to start selecting best heads. the thing is the hello protocol forces polllng frequently and one could end up syncing to the wrong head(s) (reorgs). Listing on pubsub from the start keeps the node informed about potential head changes.
- Kademlia PeerDiscovery is great too -- i mostly listed it as optional in that people don't need to implement the dht if they haven't already, but they do need to implement gossipsub and that might give them enough peer discovery. (if you have the dht, feel very able to use it. it will provide peerRouting for you too, which is a great plus. i dont think we need to require it, (maybe we do,
| - Step 2. Get the block it points to, and that block's parents | ||
| - Step 3. Graphsync the `StateTree` | ||
| - **Part 4: Catch up to the chain (`CHAIN_CATCHUP`)** | ||
| - Step 1. Maintain a set of `TargetHeads`, and select the `BestTargetHead` from it |
There was a problem hiding this comment.
- thanks -- will define. supposed to be
Blocks(which point to/define aTipset) - saw other comment, i'll think through whether this would be much better as a
Tipset(instead ofBlock) and i'll update here
| - Step 3. Graphsync the `StateTree` | ||
| - **Part 4: Catch up to the chain (`CHAIN_CATCHUP`)** | ||
| - Step 1. Maintain a set of `TargetHeads`, and select the `BestTargetHead` from it | ||
| - Step 2. Synchronize to the latest heads observed, validating blocks towards them |
There was a problem hiding this comment.
- Quantizing +1 -- let me review and merge
- i'll also spec out this "log skip list" point too. it's surprisingly simple and surprisingly useful. we can decide together whether to use it or not based on impl time benefits/costs
| - **Part 4: Catch up to the chain (`CHAIN_CATCHUP`)** | ||
| - Step 1. Maintain a set of `TargetHeads`, and select the `BestTargetHead` from it | ||
| - Step 2. Synchronize to the latest heads observed, validating blocks towards them | ||
| - Step 3. As validation progresses, `TargetHeads` and `BestTargetHead` will likely change |
There was a problem hiding this comment.
Both:
- validation can show some heads are bad heads (pruned out, maybe our
BestTargetHeadis no longer good) - time passes, node is receiving new, better candidate heads, either building on top of the previous one, or reorgs
(will clarify)
| func (c *NaiveChainsync) Sync() { | ||
| // for each of the blocks we see in the gossip channel | ||
| for block := range c.BlockPubsub.NewBlocks() { | ||
| c.IpldStore.Add(block) |
There was a problem hiding this comment.
Yes -- i still need to think through how much we want/can do at the BlockPubsub level (see the Progressive Block Validation section below)
| // ToValidateG is the set of unvalidated Blocks fully connected | ||
| // to the validated graph. These are ready to be validated, all the | ||
| // data is local, and are ordered by total ChainWeight. | ||
| ToValidateG PartialGraph |
There was a problem hiding this comment.
- ah, the block headers declares its weight, though this isn't validated yet.
Yeah, you can't trust it, but you can use it. (would be awesome to have a publicly verifiable proof there... would make everything easier...)
FetchedG
SGTM! will fix
| // Obviously bad heads SHOULD be pruned out: | ||
| // - Clearly invalid ChainWeight (lower than BestHead or too high) | ||
| // - Heads that are not advancing in BlockPubsub | ||
| TargetHeads [block.BlockCID] |
There was a problem hiding this comment.
- Yes -- sorry for the ambiguous word (head). maybe i should switch to
TargetBlocksandBestTargetBlock- i can also rename in terms of
Tipset, but thinking/dealing with them as tipsets is a bit trickier (combinatorics...)
- i can also rename in terms of
- I believe it is equivalent to do it in terms of Blocks (because each block uniquely defines a tipset, in the prior epoch, like the StateTree, and merely proposes messages for the next round) -- and it's easier to reason/program -- but this is flexible, can opt for what we prefer most
-
I think we might need a new word to refer to blocks that area at the fringe (have no children), distinct from tipsets that can be composed from those blocks.
- yes, i was using "Head" that way -- maybe
HeadBlockvsHeadTipset? hmmm open to other suggestions
|
|
||
| // Remove deletes given BlockCID from the PartialGraph, updating | ||
| // Blocks, Roots, and Leaves as needed. | ||
| Remove(c block.BlockCID) |
There was a problem hiding this comment.
is a graph necessarily connected, or is this jut a bag of blocks, with each disconnected subgraph being reported in heads/tails
the latter -- these are 4 examples of ParialGraphs, the 3rd and 4th case are separate chains.
| Heads [block.BlockCID] | ||
|
|
||
| // Tails is the set of leaf/tail blocks | ||
| Tails [block.BlockCID] |
There was a problem hiding this comment.
yeah this is an annoying nomenclature. this partial graph is a DAG fragment-- including disconnected components. here's a visual example, 4 example PartialGraphs, with Heads and Tails. (note they aren't tipsets)
| // from here, we may consider many tipsets that _include_ Block | ||
| // but we must indeed include t and not consider tipsets that | ||
| // fork from Block.Parents, but do not include Block. | ||
| type Checkpoint BlockCID |
There was a problem hiding this comment.
-
The reason a block is OK is that it uniquely identifies a tipset.
-
using tipsets directly would make Checkpoints harder to communicate. we want to make checkpoints a single hash, as short as we can have it. They will be shared in tweets, URLs, emails, printed into newspapers, etc. Compactness, ease of copy-paste, etc matters.
-
we'll make human readable lists of checkpoints, and making "lists of lists" is much more annoying. compare:
> cat block-checkpoints bafy2bzacedkf2yx3l7qmjklhylug64v2wnzw2punqiazcldtdkztq6x4745bm bafy2bzacecqgqed3x4atzznobctanup4cvkutiwdrlw7ujyl56k64yn3da4jm bafy2bzacecmqjutm36dq6kwzwuzstsagplx6rjunikpnnopocxcq5uxgby6rg bafy2bzaceccbbedox2bdf5qw2owdgurf5xrgbd6fthyt3menb6qxkgrdo7tza # vs delimiting a list of lists (space, commas, json arrays...) > cat tipset-checkpoints bafy2bzacedkf2yx3l7qmjklhylug64v2wnzw2punqiazcldtdkztq6x4745bm bafy2bzacecqgqed3x4atzznobctanup4cvkutiwdrlw7ujyl56k64yn3da4jm bafy2bzacecmqjutm36dq6kwzwuzstsagplx6rjunikpnnopocxcq5uxgby6rg bafy2bzaceccbbedox2bdf5qw2owdgurf5xrgbd6fthyt3menb6qxkgrdo7tza, bafy2bzacedkf2yx3l7qmjklhylug64v2wnzw2punqiazcldtdkztq6x4745bm -
When we have EC.E_PARENTS > 5 or = 10, tipsets will get annoyingly large.
-
the big quirk/weirdness with blocks it that it also must be in the chain. (if you relaxed that constraint you could end up in a weird case where a checkpoint isnt in the chain and that's weird/violates assumptions -- eg below).
|
Thanks, the responses clear up a few things, one being the mechanics of finality. I think this could benefit from a brief exposition at the start explaining the intentions behind the finality tipset, best heads, various graph bits etc. |
|
@jbenet here is a question that came up from discussion with @anorth: Imagine we go with the minimal This pubsub message will be propagated before fetching the header per the validation conditions. This means that peers can not be sure that their last-hop gossipsub peer actually has the header data that they are going to ask to fetch. In fact they should expect that this peer won't have that data available on graphsync. The The question is: should we fetch the header for a |
hello can stay -- still useful to query for this info, like
|
Answers to @ZenGround0
good to hear :)
yep
Ok, will flesh out. It should be fetching block headers first, connect the graph, then fetch messages to do full validation. (but will spell this out in doc)
Right
Yes, those data structures are for a
I think this can work in practice, given that:
It is safest, though, to advance in steps towards the targets.
No-- should only listen. (unless this is required from gossipsub assumptions)
I can write out the algorithm in mind in spec code, and that will force it all out. but i need to land some other higher prio stuff first. Can you ask more questions of what is underspecified to make sure i write it out in prose here
Sounds good 👍 |
I believe all of the above is correct
The main reason is not speed, but wasted bandwidth. Getting it down to something that fits in 1 packet is nice for reducing duplicate bandwidth use. If you imagine that you will receive the header once per gossipsub peer (or if lucky, half of them), and that there is 2MB in <5s may not be worth saving-- and maybe gossipsub can be much better about supressing dups. Important: I am OK to not do this until we find it to be a problem -- ie skip step 1 and step 2 for testnet/mainnet, and propagate |
|
@ZenGround0 to Q in #fil-specs:
I think though the right order of implementation (for Progressive Block Propagation) is:
if we see bandwidth being a problem in testnet, we can diagnose and decide to bring back either. I will (in the meantime) spec out and test (will update PR soon) |
Thanks. I see great utility in speccing and implementing something simple until we demonstrate (research or implementation) a problem that needs a more complex solution. In this case, I would request that the spec today describe only what we currently expect implementations to align on for/during testnet. It's fine to note that operational experience may require a change before (or after some time during) mainnet. I would prefer the other options be demoted to an appendix, if not a gist or issue, just so that there's no confusion about what the immediately-expected behaviour is. |
Ok SGTM. will make sure it shows right. (i may find a way to comment it out in the markdown so it stays close to where it's supposed to be) |
We can totally do this. The simplest form would be an RPC protocol with two operations:
|
| We expect that many nodes will synchronize blocks this way, or at least try it first, but we note | ||
| that such implementations will not be synchronizing securely or compatibly, and that a complete | ||
| implementation must include the full `ChainSync` protocol as described here. |
| - **Part 4: Catch up to the chain (`CHAIN_CATCHUP`)** | ||
| - Step 1. Maintain a set of `TargetHeads`, and select the `BestTargetHead` from it | ||
| - Step 2. Synchronize to the latest heads observed, validating blocks towards them | ||
| - Step 3. As validation progresses, `TargetHeads` and `BestTargetHead` will likely change |
| - Step 2. Get the block it points to, and that block's parents | ||
| - Step 3. Graphsync the `StateTree` | ||
| - **Part 4: Catch up to the chain (`CHAIN_CATCHUP`)** | ||
| - Step 1. Maintain a set of `TargetHeads`, and select the `BestTargetHead` from it |
| - Resource expensive verification MAY be skipped at nodes' own risk | ||
| - **Part 2: Bootstrap to the network (`BOOTSTRAP`)** | ||
| - Step 1. Bootstrap to the network, and acquire a "secure enough" set of peers (more details below) | ||
| - Step 2. Bootstrap to the `BlockPubsub` channels |
| - `ChainSync` fetches `Blocks`, `Messages`, and `StateTree` through the `Graphsync` protocol. | ||
| - `ChainSync` maintains sets of `Blocks/Tipsets` in `Graphs` (see `ChainSync.id`) | ||
| - `ChainSync` gathers a list of `LatestTipsets` from `BlockPubsub`, sorted by likelihood of being the best chain (see below). |
|
|
||
| // ValidG contains all the blocks we have validated, but are not | ||
| // yet declared Final. This means all the blocks connected into the | ||
| // main blockchain, but which ValidationGraph has not yet picked. |
| // ToValidateG is the set of unvalidated Blocks fully connected | ||
| // to the validated graph. These are ready to be validated, all the | ||
| // data is local, and are ordered by total ChainWeight. | ||
| ToValidateG PartialGraph |
| // Graph graph.BlockGraph | ||
|
|
||
| // FinalityTipset contains the latest tipset declared to be final. | ||
| // This is what we use to check against. |
| // Obviously bad heads SHOULD be pruned out: | ||
| // - Clearly invalid ChainWeight (lower than BestHead or too high) | ||
| // - Heads that are not advancing in BlockPubsub | ||
| TargetHeads [block.BlockCID] |
| // Obviously bad heads SHOULD be pruned out: | ||
| // - Clearly invalid ChainWeight (lower than BestHead or too high) | ||
| // - Heads that are not advancing in BlockPubsub | ||
| TargetHeads [block.BlockCID] |
There was a problem hiding this comment.
(for now keeping word Head but clearly noting it is a block. may still switch it before PR lands.
|
Can i get a review + approval so i can merge the PR? |
|
re: general-use peer exchange protocol, would something like this work for you, @jbenet? libp2p/specs#222 |
There was a problem hiding this comment.
This is great work. Left a few notes. My two larger points are:
- let's make sure to split validation out of graphsync altogether. It's one of these parts of the spec that is bound to get defined 8 times in slightly different ways so we need a canonical "block validation" spec (I'll merge that from consensus into it thereafter).
- I think your requirement on block sync that all chains be validated before we move from
catch uptofollowis overly strong. We can weaken it at no loss in correctness.
I think your proposal of including ChainWeight sorting as part of TargetHeads and BestTargetHead makes a lot of sense. Worth expliciting that you are sorting by weight there (didn't see it).
Adding a diagram to the code files for the protocol will help a lot in making it easier to grok but yes following our discussion from some weeks back.
|
|
||
| - Called by: libp2p {{<sref chainsync>}} | ||
| - Calls on: {{<sref chain_manager>}} | ||
| - `LastCheckpoint` the last hard social-consensus oriented checkpoint that `ChainSync` is aware of. |
There was a problem hiding this comment.
a link here to concept in consensus would be nice to disambiguate what could be meant by checkpoint
There was a problem hiding this comment.
spotted below in block.id file. Should link to there!
| synchronization functionality that is critical for Filecoin security. | ||
|
|
||
| ### Rate Limiting Graphsync responses (SHOULD) |
There was a problem hiding this comment.
nice use of keyword SHOULD
| - No new blocks are reported to consumers/users of `ChainSync` yet. | ||
| - The chain state provided is the available `Blocks` and `StateTree` for all available epochs, | ||
| specially the `FinalityTipset`. |
There was a problem hiding this comment.
not sure what the "specially" here means
| - **security conditions to transition out:** | ||
| - Gaps between `ChainSync.FinalityTipset ... ChainSync.BestTargetHead` have been closed: | ||
| - All `Blocks` and their content MUST be fetched, stored, linked, and validated locally. |
There was a problem hiding this comment.
I think this condition may be too strict. That is we need only have those blocks in the heaviest chain have been f/s/l/v locally. We are going depth first i believe to drive this (though obviously some breadth will be covered so doing). As soon as the heaviest "heard about" chain is validated, you can transition out. You needn't validate all other chains in order to move forward i believe.
There was a problem hiding this comment.
"Gaps between
ChainSync.FinalityTipset ... ChainSync.BestTargetHeadhave been closed:"
this means only the chain to BestTargetHead, so i think it covers your concern.
note that if that one turns out to be invalid, need to keep validating towards the next best one, before moving to CHAIN_FOLLOW
There was a problem hiding this comment.
Yes. You're completely right. I focused on the "all blocks" but it is appropriately scoped. Thank you
| - Keep a set of gap measures: | ||
| - `BlockGap` is the number of remaining blocks to validate between the Validated blocks and `BestTargetHead`. | ||
| - (ie how many epochs do we need to validate to have validated `BestTargetHead`. does not include null blocks) |
There was a problem hiding this comment.
I'm not sure I understand the BlockGap purpose here. The idea being stop doing any progress work and just do validation if you fall behind? If so makes sense. In which case it should be a multiple of expectedNumberOfBlocksPerRound (and in that sense a proxy for epochGap in some way)
There was a problem hiding this comment.
- The idea being stop doing any progress work and just do validation if you fall behind?
yes.
- and: the difference between
BlockGapandEpochGapis null blocks. we can tolerate more of those than real blocks gap.
| be checked and used to make syncing decisions without fetching the `Messages`. | ||
|
|
||
| ## Progressive Block Validation |
There was a problem hiding this comment.
I think this section should be split out into its own md. We've had multiple issues in the past with validation being defined in a few spots. Given how key a topic it is, relevance to consensus, etc. I think it probably warrants a standalone block val section that everything else links to.
I should be touching it up wrt consensus as well.
There was a problem hiding this comment.
Specifically we should have a canonical way of specifying (ie code) how you run various validations (eg ChainEpoch) and having a standalone file in which to link to the code would be great.
There was a problem hiding this comment.
As an implementer I think it is really critical to keep this the sequencing description in the syncing section. Sequencing is a key thing that up to now has been woefully underspecified.
Validation order matters and is non-trivially sequenced along with fetching, propagating and state updating.
I'm not sure how much of the above suggestion is about spec organization so apologies if I'm misunderstanding. For example I'm totally happy if Progressive Block Valdation continues to exist with a full description of the sequence but points into those functions in the section you're proposing.
My bad confused with previous section
There was a problem hiding this comment.
I think this section should be split out into its own md. We've had multiple issues in the past with validation being defined in a few spots. Given how key a topic it is, relevance to consensus, etc. I think it probably warrants a standalone block val section that everything else links to.
I'll probably put the functions themselves into chain package (block validation only makes sense in terms of a chain), but this section will remain here, as these stages are key to understand in chainsync, and only needed split like this here. I wont put all the specifics of how to validate each thing here -- that belongs in the canonical section. This section here can point to the code + functions in the canonical section.
My bad confused with previous section
with which section? i agreed with your (now crossed out) comment
There was a problem hiding this comment.
As long as there is a single canonical detailed section dedicated to how block Val is done, I am happy.
| - in `CHAIN_CATCHUP`, if a node is receiving/fetching hundreds/thousands of `BlockHeaders`, validating signatures can be very expensive, and can be deferred in favor of other validation. (ie lots of BlockHeaders coming in through network pipe, dont want to bound on sig verification, other checks can help dump blocks on the floor faster (BV0, BV1) | ||
| - in `CHAIN_FOLLOW`, we're not receiving thousands, we're receiving maybe a dozen or 2 dozen packets in a few seconds. We receive cid w/ Sig and addr first (ideally fits in 1 packet), and can afford to (a) check if we already have the cid (if so done, cheap), or (b) if not, check if sig is correct before fetching header (expensive computation, but checking 1 sig is way faster than checking a ton). In practice likely that which one to do is dependent on miner tradeoffs. we'll recommend something but let miners decide, because one strat or the other may be much more effective depending on their hardware, on their bandwidth limitations, or their propensity to getting DOSed | ||
|
|
There was a problem hiding this comment.
We need a note on GC in chainSync. Specifically with the fact that top-X heaviest heads' blocks and graphs should be kept around until past the finality threshold. Others can be discarded, all can be discarded at finality (SHOULD)
There was a problem hiding this comment.
Also, having a set of diagrams here would be super helpful to illustrate how this all works. We should have a toy example at the top or bottom of this index file to disambiguate intent and show off a lot of things (with code remaining canon)
There was a problem hiding this comment.
We need a note on GC in chainSync. Specifically with the fact that top-X heaviest heads' blocks and graphs should be kept around until past the finality threshold. Others can be discarded, all can be discarded at finality (SHOULD)
yeah great point
Also, having a set of diagrams here would be super helpful to illustrate how this all works. We should have a toy example at the top or bottom of this index file to disambiguate intent and show off a lot of things (with code remaining canon)
what kind of diagrams would you like to see? (rough sketch for me, i'll make them later)
There was a problem hiding this comment.
The diagrams that we made when first discussing this in LA. Showing a toy example with say 10 blocks all coming in, then being worked through the pipeline into fringe and then into longest target head.
So some set of colored blocks being validated and classified over multiple steps allowing us to see the sequence of how "sync" should work.
| `ChainSync` takes `LastCheckpoint` on faith, and builds on it, never switching away from its history. | ||
| - `TargetHeads` a list of `BlockCIDs` that represent blocks at the fringe of block production. | ||
| These are the newest and best blocks `ChainSync` knows about. They are "target" heads because | ||
| `ChainSync` will try to sync to them. This list is sorted by "likelihood of being the best chain". |
There was a problem hiding this comment.
Explicitly state that this is in terms of chainWeight, as calculated thanks to the StoragePowerConsensus subsystem
There was a problem hiding this comment.
fine to use ChainWeight now, but I think this will end up being more than ChainWeight -- for example there's some calculation about "likelihood of being correct" -- eg if the ChainWeight looks outlandish, or ChainEpoch seems off by a small factor (ie, may be right, but seems a bit off), or MinerAddress belongs to a miner which has made a number of mistakes/attacks
Maybe -- i'll keep it here for now and reconsider later. most of block validity only makes sense in terms of a chain. some functions can be defined in
No, cannot. without validating all the way to the best head, can fall into the wrong fork. consider two |
Commit includes: - summary of ChainSync - in-depth protocol prose for ChainSync - ChainSync FSM states prose - ChainSync FSM diagram - Progressive Block Validation WIP - Progressive Block Propagation WIP - Graphsync recommendations - Gossipsub recommendations - lip2p recommendations (PeerDiscovery)
|
Rebased |
Yep, it does! that would be great |
|
We're on the same page re chain sync. On the block validation section split. I remain a bit worried that the meat of how validation happens and what it should cover is useful enough across various parts of the protocol that I'd want it split out and have chain sync point to that... But let's merge and we'll amend if it indeed becomes a problem. |
Warning: this PR is controversial, and very WIP. Please read with that in mind/context.
Much of this can be reduced in scope, but i needed to put this out there as is to cover the extensive complexity and security requirement here, so that we're all on the same page, and that if we drop something, we do so with very open eyes together.
Please see the rendering here:
https://ipfs.io/ipfs/QmZ4CmrdLEiGXC2hfF7uFAj3qkHCfKgzqCPc3VnGcTKbQm/#systems__filecoin_blockchain__chainsync
7102105 (jbenet, 9 hours ago)
chainsync fsm diagram
d9f2dc8 (jbenet, 15 hours ago)
blockchain/ChainSync: wrote up ChainSync, w/ FSM
Commit includes:
7a1272b (jbenet, 30 hours ago)
mining: fsm diagrams