feat: add peer block filter option #549
Conversation
|
Thank you for submitting this PR!
Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. |
|
@aschmahmann we talked about this feature during ip stewart colo, May I ask for your guidance on how to finish this? |
laurentsenta
force-pushed
the
feat/peer-block-filter
branch
from
91c3c55
to
3959a14
Compare
|
Thanks for running the workflow, I just rebased on master, but the previous run is here. Everything passed, except the TestSessionFailingToGetFirstBlock. edit: |
laurentsenta
force-pushed
the
feat/peer-block-filter
branch
from
3959a14
to
dd1e961
Compare
laurentsenta
force-pushed
the
feat/peer-block-filter
branch
from
962334f
to
c24da0b
Compare
There was a problem hiding this comment.
I actually really like the way you broke out denials. I agree that altering the wantKs to later control what blocks are not found feels weird. blockSizes and wants would not have been in sync.
I don't mind the anonymous function. Reworking it so that those scoped variables are altered in a function feels worse in my opinion. I'd rather have this.
This feature lets a user configure a function that will allow / deny request for a block coming from a peer.
Split the different cases (wants, cancel, denies) early to prevent calling blocksize on rejected blocks.
laurentsenta
force-pushed
the
feat/peer-block-filter
branch
from
c24da0b
to
f3187ff
Compare
| } | ||
| e.MessageReceived(context.Background(), partner, add) | ||
| } | ||
|
|
||
| func partnerWantBlocksHaves(e *Engine, keys []string, wantHaves []string, sendDontHave bool, partner peer.ID) { | ||
| func partnerWantBlocksHaves(e *Engine, wantBlocks []string, wantHaves []string, sendDontHave bool, partner peer.ID) { |
There was a problem hiding this comment.
rationale: I didn't understand this call at first so I made the parameters name symmetric, wantHave <-> wantBlocks.
Which lead me to rename the parameter in the other function above (since it was probably a copy and paste).
laurentsenta
force-pushed
the
feat/peer-block-filter
branch
2 times, most recently
from
e151ec0
to
dbe1555
Compare
laurentsenta
force-pushed
the
feat/peer-block-filter
branch
from
6a88c10
to
22a8afe
Compare
laurentsenta
force-pushed
the
feat/peer-block-filter
branch
from
22a8afe
to
44432bc
Compare
| allowList: "c", | ||
| wantBlks: "bc", // Note: We request a block here to force a response from the node | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Note the detail here: when a node sends a want-[have/block], if the response is positive (we have the block), sending the same request for the same block will get ignored.
That make sense in practice
but in testing, if a node loses access to a piece of data, it's /hard/ to test, that's why here we switch from want-have to want-block.
|
2022-03-03 verbal: lets create an issue that explains what a user will get when this is all done. I assume there will be a checklist for the go-bitswap side and plumbing it through to go-ipfs. Great work! |
|
Created issue here: ipfs/kubo#8763 |
| keys := []string{"a", "b", "c", "d"} | ||
| blks := make([]blocks.Block, 0, len(keys)) | ||
| for _, letter := range keys { | ||
| block := blocks.NewBlock([]byte(letter)) |
There was a problem hiding this comment.
For testing this is fine, but wanted to give you a heads up that this is not really a valid IPLD block since it's creating a CIDv0 (which must be dag-pb) with the value "a".
Instead I'd use NewBlockWithCid and pass it a CIDv1 with the Raw codec
This feature lets a user configure a filter to allow / deny request according to the request's peer ID and the content id.
For example, a user may use this option to implement a dynamic peer-based authorization.