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(routing): allow-offline with routing put #278

Merged
merged 4 commits into from
May 3, 2023
Merged

Conversation

laurentsenta
Copy link
Contributor

@laurentsenta laurentsenta commented Apr 12, 2023

sister PR in kubo: ipfs/kubo#9667

@laurentsenta laurentsenta requested a review from a team as a code owner April 12, 2023 09:11
@laurentsenta laurentsenta marked this pull request as draft April 12, 2023 09:11
@codecov
Copy link

codecov bot commented Apr 12, 2023

Codecov Report

Merging #278 (b813722) into main (268cad8) will decrease coverage by 0.06%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #278      +/-   ##
==========================================
- Coverage   47.88%   47.82%   -0.06%     
==========================================
  Files         273      274       +1     
  Lines       33186    33237      +51     
==========================================
+ Hits        15890    15895       +5     
- Misses      15616    15665      +49     
+ Partials     1680     1677       -3     
Impacted Files Coverage Δ
coreiface/options/routing.go 0.00% <0.00%> (ø)
coreiface/tests/api.go 0.00% <0.00%> (ø)
coreiface/tests/dht.go 0.00% <0.00%> (ø)
coreiface/tests/name.go 0.00% <0.00%> (ø)
coreiface/tests/pubsub.go 0.00% <0.00%> (ø)
coreiface/tests/routing.go 0.00% <0.00%> (ø)

... and 9 files with indirect coverage changes

@Jorropo
Copy link
Contributor

Jorropo commented Apr 13, 2023

Is ipfs/kubo#9667 correct ? I don't see how this relates

@laurentsenta
Copy link
Contributor Author

@Jorropo yup: ipfs/kubo@290f29a

@BigLep
Copy link
Contributor

BigLep commented May 1, 2023

@hacdias : can you please look at this again so we can get it merged?

Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @laurentsenta. This looks good to me.

I would like to see a test for this in coreiface/tests/routing.go but I'm not sure how easy that would be. I think by using tp.MakeAPISwarm(ctx, true, 1) you'll be able to spin an offline node and therefore Put should work with AllowOffline(true) but not without. Note that this tests are run in Kubo's repo.

@laurentsenta
Copy link
Contributor Author

laurentsenta commented May 2, 2023

@hacdias thanks for taking the time to review both PRs,

From a quick look + tests:

  • There is some coupling between online and full identity in the node init

I'll dig some more, but let me know if you've seen this before: when I try to start a node with full identity AND offline, I get a sigsev

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x40 pc=0x10131ab08]

goroutine 773 [running]:
github.com/ipfs/kubo/core/bootstrap.bootstrapRound({0x102732a98?, 0x14000a88640?}, {0x0, 0x0}, {0x14000315fb0?, 0x0?, 0x140003a6330?, 0x14000322fe0?})
	/Users/laurent/dev/plabs/ipfs/kubo/core/bootstrap/bootstrap.go:118 +0x68
github.com/ipfs/kubo/core/bootstrap.Bootstrap.func1({0x102741478?, 0x140004f5320?})
	/Users/laurent/dev/plabs/ipfs/kubo/core/bootstrap/bootstrap.go:89 +0xac
github.com/jbenet/goprocess.(*process).Go.func1()
	/Users/laurent/dev/go/pkg/mod/github.com/jbenet/goprocess@v0.1.4/impl-mutex.go:134 +0x3c
created by github.com/jbenet/goprocess.(*process).Go
	/Users/laurent/dev/go/pkg/mod/github.com/jbenet/goprocess@v0.1.4/impl-mutex.go:133 +0x204
FAIL	github.com/ipfs/kubo/core/coreapi/test	0.856s
FAIL
  • Currently, the "sane" way to create an IPNS entry is by going through Publish + Get (that's what the test for Put does); that might be the right time to extract that code from the Publish function. We had a similar discussion about verify and unmarshall.

@hacdias
Copy link
Member

hacdias commented May 2, 2023

@laurentsenta I've never seen this error before, and it might be a bug? It seems like host is nil, which seems to make sense since we do not set PeerHost in IpfsNode. It seems like we'll need to make a few changes to the test function in Kubo to ensure we can have an offline and full identity node. But first, do we need full identity here?

Currently, the "sane" way to create an IPNS entry is by going through Publish + Get (that's what the test for Put does); that might be the right time to extract that code from the Publish function.

Do you mean a Create function?

coreiface/tests/routing.go Outdated Show resolved Hide resolved
@hacdias hacdias merged commit 8059f18 into main May 3, 2023
@hacdias hacdias deleted the feat/ipns-allow-offline branch May 3, 2023 10:59
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
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants