Skip to content
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

Conversation

SgtPooki
Copy link
Member

  • feat: blockBrokers can control block validation

Built on top of #281 & #284

This PR adds the validateFn option to BlockRetriever.retrieve options and slightly modifies
how raceBlockRetrievers inside of NetworkedStorage is handled. Some callouts:

  • BlockBroker's are given a TRUSTED validateFn to call when they retrieve blocks, and MAY call this method prior to returning the block
  • If they call validateFn, when
    • it doesn't throw, we consider the blocks to have been verified (since we own the inner validateFn and given MultihashHashers)
    • it throws, they can handle recourse within their retrieve method (e.g. try other gateways, other peers/transports/etc, whatever they want)
      • If they do not, we consider that blockBroker to have failed the retrieve method and other BlockBrokers will continue to raceBlockRetrievers as expected
  • If they do not call validateFn, we will call it for them, and then
    • If it doesn't throw, we consider the blocks to have been verified (since we own the inner validateFn and given MultihashHashers)
    • If it throws, we consider that blockBroker to have failed the retrieve method and other BlockBrokers will continue to raceBlockRetrievers as expected
  • The types of the retrieve method in BitswapBlockBroker were modified so a validateFn 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.

@SgtPooki SgtPooki requested a review from a team as a code owner October 10, 2023 21:33
@SgtPooki SgtPooki changed the base branch from main to fix/block-broker-testing October 10, 2023 21:34
Copy link
Member Author

@SgtPooki SgtPooki left a 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.

Comment on lines +235 to 245
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)
}
Copy link
Member Author

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 blockBrokers 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)

@SgtPooki SgtPooki merged commit cc3e9ce into fix/block-broker-testing Oct 13, 2023
15 checks passed
@SgtPooki SgtPooki deleted the fix/block-broker-testing-invert-blockValidation-control branch October 13, 2023 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

1 participant