search

ethereum / trinity

The Trinity client for the Ethereum network
https://trinity.ethereum.org
MIT License
474 stars 145 forks source link

Move witness fetcher to BeamSyncService #2113

Closed gsalgado closed 3 years ago

gsalgado commented 3 years ago

For example, seeing a BlockWitnessResult makes me think that "we have collected all the state necessary to import the block." But I think the event actually means something like "The hashes needed to collect the state for a block are available."

The BlockWitnessResult event actually means we have collected all the state necessary for the block, although only the meta-witness is included in the event. This is something that changed in this PR as before we were letting the trie nodes be fetched in the background, after returning the witness hashes, whereas now the _fetch_witness() function waits for the trie nodes so when it returns we have actually fetched the block witness.

There are a number of other places where "witness" could be switched to "meta-witness." (or "witness hashes" if you like that better)

I remember you suggested that in a previous PR and I tried to keep it consistent on this one. I think the confusion here was because it was not obvious that the witness-fetching APIs now wait for the trie nodes as well

gsalgado commented 3 years ago

@carver does https://github.com/ethereum/trinity/pull/2113/commits/4ed31be86b2a8c84e92d725635e30dbdb067790a look good?

carver commented 3 years ago

Yup, looks good! (Apart from aggravating CI failures)