-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
feat(config): ability to disable Bitswap fully or just server #10782
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
Open
gystemd
wants to merge
7
commits into
ipfs:master
Choose a base branch
from
gystemd:feat/bitswap/disable
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
14c7ccc
feat: add Bitswap configuration and related tests
gystemd 682b47b
fix: update Bitswap function to use 'provide' parameter for server en…
gystemd a2c34bd
Merge branch 'master' into feat/bitswap/disable
gystemd 366b56e
docs: update changelog for Bitswap functionality changes
gystemd bc4a386
Merge branch 'bitswap-disable' into feat/bitswap/disable
gystemd 411ed04
fix: update Bitswap server enablement logic and improve related tests
gystemd 7c6b791
fix: rename BitswapConfig to Bitswap and update references
gystemd File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
package config | ||
|
||
// Bitswap holds Bitswap configuration options | ||
type Bitswap struct { | ||
// Enabled controls both client and server (enabled by default) | ||
Enabled Flag `json:",omitempty"` | ||
// ServerEnabled controls if the node responds to WANTs (depends on Enabled, enabled by default) | ||
ServerEnabled Flag `json:",omitempty"` | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,136 @@ | ||
package cli | ||
|
||
import ( | ||
"testing" | ||
"time" | ||
|
||
"github.com/ipfs/kubo/config" | ||
"github.com/ipfs/kubo/test/cli/harness" | ||
"github.com/ipfs/kubo/test/cli/testutils" | ||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestBitswapConfig(t *testing.T) { | ||
t.Parallel() | ||
|
||
// Create test data that will be shared between nodes | ||
testData := testutils.RandomBytes(100) | ||
|
||
t.Run("server enabled (default)", func(t *testing.T) { | ||
t.Parallel() | ||
h := harness.NewT(t) | ||
provider := h.NewNode().Init().StartDaemon() | ||
requester := h.NewNode().Init().StartDaemon() | ||
|
||
hash := provider.IPFSAddStr(string(testData)) | ||
requester.Connect(provider) | ||
|
||
res := requester.IPFS("cat", hash) | ||
assert.Equal(t, testData, res.Stdout.Bytes(), "retrieved data should match original") | ||
}) | ||
|
||
t.Run("server disabled", func(t *testing.T) { | ||
t.Parallel() | ||
h := harness.NewT(t) | ||
|
||
provider := h.NewNode().Init() | ||
provider.SetIPFSConfig("Bitswap.ServerEnabled", false) | ||
provider = provider.StartDaemon() | ||
|
||
requester := h.NewNode().Init().StartDaemon() | ||
|
||
hash := provider.IPFSAddStr(string(testData)) | ||
requester.Connect(provider) | ||
|
||
// If the data was available, it would be retrieved immediately. | ||
// Therefore, after the timeout, we can assume the data is not available | ||
// i.e. the server is disabled | ||
timeout := time.After(3 * time.Second) | ||
dataChan := make(chan []byte) | ||
|
||
go func() { | ||
res := requester.RunIPFS("cat", hash) | ||
dataChan <- res.Stdout.Bytes() | ||
}() | ||
|
||
select { | ||
case data := <-dataChan: | ||
assert.NotEqual(t, testData, data, "retrieved data should not match original") | ||
case <-timeout: | ||
t.Log("Test passed: operation timed out after 3 seconds as expected") | ||
} | ||
}) | ||
|
||
t.Run("server disabled and client enabled", func(t *testing.T) { | ||
t.Parallel() | ||
h := harness.NewT(t) | ||
|
||
provider := h.NewNode().Init() | ||
provider.SetIPFSConfig("Bitswap.ServerEnabled", false) | ||
provider.StartDaemon() | ||
|
||
requester := h.NewNode().Init().StartDaemon() | ||
hash := requester.IPFSAddStr(string(testData)) | ||
|
||
provider.Connect(requester) | ||
|
||
// Even when the server is disabled, the client should be able to retrieve data | ||
res := provider.RunIPFS("cat", hash) | ||
assert.Equal(t, testData, res.Stdout.Bytes(), "retrieved data should match original") | ||
}) | ||
|
||
t.Run("bitswap completely disabled", func(t *testing.T) { | ||
t.Parallel() | ||
h := harness.NewT(t) | ||
|
||
requester := h.NewNode().Init() | ||
requester.UpdateConfig(func(cfg *config.Config) { | ||
cfg.Bitswap.Enabled = config.False | ||
cfg.Bitswap.ServerEnabled = config.False | ||
}) | ||
requester.StartDaemon() | ||
|
||
provider := h.NewNode().Init().StartDaemon() | ||
hash := provider.IPFSAddStr(string(testData)) | ||
|
||
requester.Connect(provider) | ||
res := requester.RunIPFS("cat", hash) | ||
assert.Equal(t, []byte{}, res.Stdout.Bytes(), "cat should not return any data") | ||
|
||
// Verify that basic operations still work with bitswap disabled | ||
res = requester.IPFS("id") | ||
assert.Equal(t, 0, res.ExitCode(), "basic IPFS operations should work") | ||
res = requester.IPFS("bitswap", "stat") | ||
assert.Equal(t, 0, res.ExitCode(), "bitswap stat should work even with bitswap disabled") | ||
res = requester.IPFS("bitswap", "wantlist") | ||
assert.Equal(t, 0, res.ExitCode(), "bitswap wantlist should work even with bitswap disabled") | ||
|
||
// Verify local operations still work | ||
hashNew := requester.IPFSAddStr("random") | ||
res = requester.IPFS("cat", hashNew) | ||
assert.Equal(t, []byte("random"), res.Stdout.Bytes(), "cat should return the added data") | ||
}) | ||
|
||
// t.Run("bitswap protocols disabled", func(t *testing.T) { | ||
// t.Parallel() | ||
// harness.EnableDebugLogging() | ||
// h := harness.NewT(t) | ||
|
||
// provider := h.NewNode().Init() | ||
// provider.SetIPFSConfig("Bitswap.ServerEnabled", false) | ||
// provider = provider.StartDaemon() | ||
// requester := h.NewNode().Init().StartDaemon() | ||
// requester.Connect(provider) | ||
// // Parse and check ID output | ||
// res := provider.IPFS("id", "-f", "<protocols>") | ||
// protocols := strings.Split(strings.TrimSpace(res.Stdout.String()), "\n") | ||
|
||
// // No bitswap protocols should be present | ||
// for _, proto := range protocols { | ||
// assert.NotContains(t, proto, bsnet.ProtocolBitswap, "bitswap protocol %s should not be advertised when server is disabled", proto) | ||
// assert.NotContains(t, proto, bsnet.ProtocolBitswapNoVers, "bitswap protocol %s should not be advertised when server is disabled", proto) | ||
// assert.NotContains(t, proto, bsnet.ProtocolBitswapOneOne, "bitswap protocol %s should not be advertised when server is disabled", proto) | ||
// assert.NotContains(t, proto, bsnet.ProtocolBitswapOneZero, "bitswap protocol %s should not be advertised when server is disabled", proto) | ||
// } | ||
// }) | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 am afraid this does not actually disable bitswap server, it only makes sure the server will always return "i dont have this blocks" because empty datastore is used.
The problem with this approach is that such node will still announce itself as bitswap server – you can see it by making
ipfs id
→Protocols
section will still have:This is a problem because such node will still receive bitswap queries ans asks for random CIDs from other peers, and will have to respond to them, effectively wasting resources.
Desired behavior
What we want for
Bitswap.ServerEnabled=false
is to completely "stop acting as bitswap server" and that means removing it from libp2p identify response (ipfs id
), so other peers won't even talk to us over bitswap on their own (and only when we connect to them, as a client).There are two ways of doing it – not sure which one will be easier, it is fine for you to investugate before making a decision:
/ipfs/bitswap*
server to other peers./ipfs/bitswap*
from libp2p identify response (which can be inspected viaipfs id <peerid>
orvole libp2p identify
)/ipfs/bitswap
to libp2p hostI'd say see if (A) is easy to do, but if you find yourself being blocked by a bigger refactor (e.g. all the commands that expect bitswap server to be always present), consider (B) instead, as it would have smaller code surface, and also be easier to maintain.
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 investigated both the suggested solutions , but I got stuck in trying to prevent the node to communicate /ipfs/bitswap* as supported protocols: the identify protocol is segregated inside the go-libp2p library and I cannot manipulate its internals. I have found a way to remove the handler associated to the /ipfs/bitswap* protocol in the Host's multiplexer (Mux). This way, the protocol is effectively removed from the list of supported protocols, but it results in the client not being able to send messages anymore (I suspect it is due to this boxo function). What if we register a stream handler for that just immediately shutsdown? I mean replacing this handler with an empty method that just closes the stream. This way maybe the node will stop responding completely to other nodes bitswap requests.
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.
@gystemd thank you for looking into this.
I wonder if we could do this earlier
bitswap/network/bsnet
– I'm thinkingStart
instead ofhandleNewStream
:SetStreamHandler
here: https://github.com/ipfs/boxo/blob/dc60fe747c375c631a92fcfd6c7456f44a760d24/bitswap/network/bsnet/ipfs_impl.go#L402-L403 then the server would never register a handlerThere 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.
thank you for you time and sorry for the confusion, but after a deeper investigation I found out that the StreamHandler seems to be mandatory. It is responsible for deliverying packets to the client. When running tests, I had crashes due to failed invocations of the handler. I added a proposal option in Boxo ipfs/boxo#911 to disable the server with related tests. Could you see if this is an acceptable solution that fit the needs?
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 pushed the new tests for Kubo, which are failing due to the reliance of a modified version of Boxo.