feat: check cids in denylist before providing #215

olizilla · 2023-06-20T21:31:31Z

cap the max number of cids we'll accept in a single message.
- We're seeing 20k spikes in our bitswap-pending-entries metrics per node, so I'm putting in a hard cap of 500 wanted cids per message that we'll process. The caller can ask again if they need more. This also means i can put a sensible cap on how many cids the denylist service should handle in a batch.
check batches of cids against our denylist api.
cache entries that are on the denylist; they are rarely removed.
use cache to avoid asking about items we already know, and as a fallback if denylist service cannot be reached.
add bitswap-denied counter metric to see how many CIDs we skip due to being on the denylist

see: batch endpoint for denylist api – storacha/reads#166
see: set DENYLIST_URL in env - elastic-ipfs/bitswap-peer-deployment#99

License: MIT

- cap the max number of cids we'll accept in a single message. - check batches of cids against our denylist api. - cache entries that are on the denylist; they are rarely removed. License: MIT Signed-off-by: Oli Evans <[email protected]>

and lint License: MIT Signed-off-by: Oli Evans <[email protected]>

License: MIT Signed-off-by: Oli Evans <[email protected]>

olizilla · 2023-06-21T11:14:11Z

src/deny.js

+    if (res.ok) {
+      const denylist = new Set(await res.json())
+      for (const cidStr of denylist.values()) {
+        denylistCache.set(cidToKey(CID.parse(cidStr)), true)


these cids are never pulled out of the cache... so items that do get removed from a badbits list won't get updated here... but our bitswap peer nodes tend to get restarted every day, so i'm not going to do extra work here... if this PR magically makes our pods less crashy, then we can review this.

could this comment be also added in the code file? I feel it would be nice to have context there, and maybe an issue created

License: MIT Signed-off-by: Oli Evans <[email protected]>

olizilla · 2023-06-22T10:57:19Z

package.json

@@ -28,6 +28,8 @@
    "libp2p": "0.42.2",
    "lru-cache": "7.14.1",
    "mnemonist": "0.39.5",
+    "multiformats": "^10.0.3",


10 because this code already depends on it at that version but previously didn't declare that dep, so would use a random one of 3 different major versions that are in the dep tree. Version 11+ doesn't work with the test helpers due to https://github.com/multiformats/js-multiformats/blob/4a36fb7ee49edb4300267b90301ef0e4300cbc46/src/cid.js#L305

alanshaw · 2023-06-22T10:55:41Z

src/deny.js

+        denylistCache.set(cidToKey(CID.parse(cidStr)), true)
+      }
+      // all `cancel`s go through, and items not on the denylist
+      return entries.filter(entry => entry.cancel || !denylist.has(entry.cid.toString()))


Just incase you ever update a previously cahced item to false...

Suggested change

return entries.filter(entry => entry.cancel || !denylist.has(entry.cid.toString()))

return entries.filter(entry => entry.cancel || !denylist.get(entry.cid.toString()))

using get here to update the last used time. In this LRU impl has tests do not update that.

ah, confusing. denylist here is the set of CIDs returned from the denylist api, that represents the CIDs that should not be served. Is a Set not a Map

elsewhere we have denylistCache which is an LRU, and requires us to store a value for a our CID key, and that has a Map-like api, but we must call .get to update the last used time. I will tweak the names to make clearer.

vasco-santos · 2023-06-26T07:40:15Z

src/config.js

-    allowReadinessTweak: process.env.ALLOW_READINESS_TWEAK === 'true'
+    allowReadinessTweak: process.env.ALLOW_READINESS_TWEAK === 'true',
+
+    denylistUrl: process.env.DENYLIST_URL ? new URL(process.env.DENYLIST_URL) : undefined


should this be added to the .env.sample?
We also need to get this set in the deployment

vasco-santos · 2023-06-26T07:41:57Z

src/deny.js

+    if (res.ok) {
+      const denylist = new Set(await res.json())
+      for (const cidStr of denylist.values()) {
+        denylistCache.set(cidToKey(CID.parse(cidStr)), true)


could this comment be also added in the code file? I feel it would be nice to have context there, and maybe an issue created

Send an array of CID strings to check if they are on the denylist. Response is the subset of the array that is on the denylist. ```http POST / Content-Type: application/json ["cid1","cid2"] ``` This will be used by e-ipfs to check a batch of cids at once, see: elastic-ipfs/bitswap-peer#215 License: MIT --------- Signed-off-by: Oli Evans <[email protected]>

License: MIT Signed-off-by: Oli Evans <[email protected]>

olizilla · 2023-07-05T14:17:54Z

@vasco-santos @alanshaw this PR changed a little since you last looked so please take a look if you have a moment.

added bitswap-denied counter metric to track how many CIDs we ignore due to being on the denylist
made the logic in deny.js more readable, particularly around the nuances of how we use the cached info vs the current denylist we get back from the api
simplified the cache keys to just use cid strings. trades off catching alt cid formulations at the cache layer for simplicity and not having to cid.parse every item in the response list to dig out the multihash.

Ensure DENYLIST_URL is set in env. see: elastic-ipfs/bitswap-peer#215 License: MIT Signed-off-by: Oli Evans <[email protected]>

see: elastic-ipfs/bitswap-peer#215 License: MIT Signed-off-by: Oli Evans <[email protected]>

Ensure DENYLIST_URL is set in env when deploying bitswap-peer see: elastic-ipfs/bitswap-peer#215 License: MIT --------- Signed-off-by: Oli Evans <[email protected]>

see: elastic-ipfs/bitswap-peer#215 License: MIT Signed-off-by: Oli Evans <[email protected]>

olizilla added 5 commits June 20, 2023 22:30

feat: check cids in denylist before providing

22a8de6

- cap the max number of cids we'll accept in a single message. - check batches of cids against our denylist api. - cache entries that are on the denylist; they are rarely removed. License: MIT Signed-off-by: Oli Evans <[email protected]>

feat: denylistFilter test

5287f3b

and lint License: MIT Signed-off-by: Oli Evans <[email protected]>

chore: add test for truncateWantlist

b4ec8d7

License: MIT Signed-off-by: Oli Evans <[email protected]>

fix: test and fix fallback to denylist cache

27752c1

License: MIT Signed-off-by: Oli Evans <[email protected]>

chore: lint

b3613c3

License: MIT Signed-off-by: Oli Evans <[email protected]>

olizilla marked this pull request as ready for review June 21, 2023 11:09

olizilla requested review from vasco-santos and alanshaw June 21, 2023 11:09

olizilla commented Jun 21, 2023

View reviewed changes

olizilla added 2 commits June 21, 2023 19:09

chore: fix tests

987da8b

License: MIT Signed-off-by: Oli Evans <[email protected]>

chore: explicit dep on multiformats

e444048

License: MIT Signed-off-by: Oli Evans <[email protected]>

olizilla mentioned this pull request Jun 21, 2023

feat: add batch denylist check storacha/reads#166

Merged

olizilla commented Jun 22, 2023

View reviewed changes

alanshaw approved these changes Jun 22, 2023

View reviewed changes

vasco-santos approved these changes Jun 26, 2023

View reviewed changes

olizilla added 4 commits July 5, 2023 14:04

feat: add prom counter for bitswap-denied for cids found on denylist

6ae2c87

License: MIT Signed-off-by: Oli Evans <[email protected]>

chore: lint

096816d

License: MIT Signed-off-by: Oli Evans <[email protected]>

chore: rework to be clearer

89e6270

License: MIT Signed-off-by: Oli Evans <[email protected]>

chore: add DENYLIST_URL to sample env

272712f

License: MIT Signed-off-by: Oli Evans <[email protected]>

olizilla requested review from alanshaw and vasco-santos July 5, 2023 14:14

olizilla deployed to dev July 5, 2023 14:35 — with GitHub Actions Active

alanshaw approved these changes Jul 5, 2023

View reviewed changes

olizilla added a commit to elastic-ipfs/bitswap-peer-deployment that referenced this pull request Jul 5, 2023

feat: set DENYLIST_URL env var

dd6aae0

Ensure DENYLIST_URL is set in env. see: elastic-ipfs/bitswap-peer#215 License: MIT Signed-off-by: Oli Evans <[email protected]>

olizilla mentioned this pull request Jul 5, 2023

feat: set DENYLIST_URL env var for bitswap-peer elastic-ipfs/bitswap-peer-deployment#99

Merged

olizilla merged commit 0f0a4a3 into main Jul 5, 2023

olizilla deleted the denylist branch July 5, 2023 14:57

olizilla added a commit to elastic-ipfs/bitswap-peer-deployment that referenced this pull request Jul 5, 2023

chore: deploy denylist check to staging

5b73fce

see: elastic-ipfs/bitswap-peer#215 License: MIT Signed-off-by: Oli Evans <[email protected]>

olizilla added a commit to elastic-ipfs/bitswap-peer-deployment that referenced this pull request Jul 6, 2023

chore: deploy denylist support to prod

276d16a

see: elastic-ipfs/bitswap-peer#215 License: MIT Signed-off-by: Oli Evans <[email protected]>

olizilla mentioned this pull request Jul 6, 2023

chore: deploy denylist support to prod elastic-ipfs/bitswap-peer-deployment#100

Merged

olizilla added a commit to elastic-ipfs/bitswap-peer-deployment that referenced this pull request Jul 6, 2023

chore: deploy denylist support to prod (#100)

55bc3a8

see: elastic-ipfs/bitswap-peer#215 License: MIT Signed-off-by: Oli Evans <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: check cids in denylist before providing #215

feat: check cids in denylist before providing #215

olizilla commented Jun 20, 2023 •

edited

Loading

olizilla Jun 21, 2023

vasco-santos Jun 26, 2023

olizilla Jun 22, 2023

alanshaw Jun 22, 2023

olizilla Jun 22, 2023

olizilla Jul 5, 2023

vasco-santos Jun 26, 2023

vasco-santos Jun 26, 2023

olizilla commented Jul 5, 2023

	return entries.filter(entry => entry.cancel \|\| !denylist.has(entry.cid.toString()))
	return entries.filter(entry => entry.cancel \|\| !denylist.get(entry.cid.toString()))

feat: check cids in denylist before providing #215

feat: check cids in denylist before providing #215

Conversation

olizilla commented Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olizilla commented Jul 5, 2023

olizilla commented Jun 20, 2023 •

edited

Loading