search

ethereum / trinity

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

Do not overwrite existing witness hashes for a given block in the DB #2105

Closed gsalgado closed 3 years ago

gsalgado commented 3 years ago

Hm, just realized/noticed that we do not neatly handle what happens if we get different answers from different peers. We just overwrite the last witness-hashes at the same block. This would be especially bad if the second peer to send us the witness hashes gives us a list with one hash (to grief us). We also append the block to the witness history, so our history gets shorter than intended...

This might be a problem soonish, but I think it can be all handled in the witness db (storing and reading a union of all the hashes instead of overwriting the previous), so I don't think it needs to be fixed in this PR.

_Originally posted by @carver in https://github.com/ethereum/trinity/pull/2103#discussion_r522516017_

carver commented 3 years ago

I can take a hack at this

carver commented 3 years ago

So we need to be sure to handle any race conditions, like two hash sets being saved at roughly the same time, and they both read out an empty set of hashes for a given blockhash. So might need a lock.

Current plan is to return the union of all hashes reported by all peers. It's probably worse to collect not enough data than to collect too much. Someday we might have to deal with griefing by sending way too many hashes, but for now I think empty witness griefing is a bigger concern.

Someday we might want something like a Counter() of the hashes returned by all peers, but for now I think a simple union of the set is the best next step.