-
Notifications
You must be signed in to change notification settings - Fork 110
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
feat: blockBrokers can control block validation #285
feat: blockBrokers can control block validation #285
Conversation
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.
self review.. things look good.
validateFn: async (block: Uint8Array): Promise<void> => { | ||
await validateFn(block) | ||
blocksWereValidated = true | ||
} | ||
}) | ||
|
||
// verify block | ||
const hash = await hasher.digest(block) | ||
|
||
if (!uint8ArrayEquals(hash.digest, cid.multihash.digest)) { | ||
throw new CodeError('Hash of downloaded block did not match multihash from passed CID', 'ERR_HASH_MISMATCH') | ||
if (!blocksWereValidated) { | ||
// the blockBroker either did not throw an error when attempting to validate the block | ||
// or did not call the validateFn at all. We should validate the block ourselves | ||
await validateFn(block) | ||
} |
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.
We could have a callback method where we tell blockBroker
s that they gave us an invalid block, and then wait for them to handle it however they see fit, but that would require us to do another try block, and ends up in some weird "back-and-forth" control scenario when blockProviders have complex retry flows.
We don't want to get in the business of handling those retry flows, so exposing a function they can call to determine whether blocks are valid or not prior to giving control back to us is the most simple & flexible method I can think of right now.
We can adjust if we see problems with this, but I think this is fairly flexible, and won't end up with duplicate validations (except where js-ipfs-bitswap is doing so internally)
Built on top of #281 & #284
This PR adds the
validateFn
option toBlockRetriever.retrieve
options and slightly modifieshow
raceBlockRetrievers
inside ofNetworkedStorage
is handled. Some callouts:validateFn
to call when they retrieve blocks, and MAY call this method prior to returning the blockvalidateFn
, whenvalidateFn
and given MultihashHashers)retrieve
method (e.g. try other gateways, other peers/transports/etc, whatever they want)retrieve
method and otherBlockBrokers
will continue toraceBlockRetrievers
as expectedvalidateFn
, we will call it for them, and thenvalidateFn
and given MultihashHashers)retrieve
method and otherBlockBrokers
will continue toraceBlockRetrievers
as expectedretrieve
method inBitswapBlockBroker
were modified so avalidateFn
can be handled there, but no work towards that effort should be done until feat: stop automatically verifying bytes js-ipfs-bitswap#603 is handled.