-
Notifications
You must be signed in to change notification settings - Fork 129
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
Fluffy: Portal subnetwork peer ban list #3007
base: master
Are you sure you want to change the base?
Changes from all commits
1f7869b
000e2bb
db11fb1
4b8cf1c
5f91b1b
5e38c95
22d4627
aed4020
0ca892a
628285f
89d7230
885a1c0
4b855f1
279e2eb
287262b
915cab2
b2f2784
0d73e57
ac109f0
e335374
120ef39
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,6 +46,7 @@ proc installPortalStateApiHandlers*(rpcServer: RpcServer, p: PortalProtocol) = | |
of Content: | ||
let valueBytes = foundContentResult.content | ||
validateRetrieval(key, valueBytes).isOkOr: | ||
p.banNode(node.id, NodeBanDurationContentLookupFailedValidation) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No huge deal, but having these There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be clear, the bans on this behaviour are correct, and should be done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah I guess at some point I could refactor this to import the state network into the rpc api and then reuse the existing functions there which will already apply the bans as needed. Probably better to do that in a future PR though. |
||
raise invalidValueErr() | ||
|
||
let res = ContentInfo( | ||
|
@@ -97,6 +98,10 @@ proc installPortalStateApiHandlers*(rpcServer: RpcServer, p: PortalProtocol) = | |
valueBytes = contentLookupResult.content | ||
|
||
validateRetrieval(key, valueBytes).isOkOr: | ||
p.banNode( | ||
contentLookupResult.receivedFrom.id, | ||
NodeBanDurationContentLookupFailedValidation, | ||
) | ||
raise invalidValueErr() | ||
p.storeContent(keyBytes, contentId, valueBytes, cacheContent = true) | ||
|
||
|
@@ -132,6 +137,10 @@ proc installPortalStateApiHandlers*(rpcServer: RpcServer, p: PortalProtocol) = | |
raise contentNotFoundErrWithTrace(data) | ||
|
||
validateRetrieval(key, valueBytes).isOkOr: | ||
if res.trace.receivedFrom.isSome(): | ||
p.banNode( | ||
res.trace.receivedFrom.get(), NodeBanDurationContentLookupFailedValidation | ||
) | ||
raise invalidValueErr() | ||
p.storeContent(keyBytes, contentId, valueBytes, cacheContent = true) | ||
|
||
|
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.
While all these banNodes are fine in theory. They might cause issues with a occasional Trin bug where only partial data is send currently.
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.
I think the longer term improvement for this would be to add counts of violations and then only ban nodes after a certain limit is reached. For this PR though I guess we could simply reduce the ban time if there are concerns with it not behaving as intended.