-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Private network implementation #3396
Conversation
ab2f1e1
to
5c07c12
Compare
@whyrusleeping it should be ready for CR. |
@@ -578,6 +581,26 @@ func (r *FSRepo) GetStorageUsage() (uint64, error) { | |||
return du, err | |||
} | |||
|
|||
func (r *FSRepo) SwarmKey() ([]byte, error) { |
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.
Shall the swarm key be part of the ipfs configuration file (as the host identity is?). My first impression is that it would be a more consistent approach.
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.
No, as we want to move the privkey out of the config for a long time already.
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.
Config is exposed via the HTTP API and right now we some ugly hacks to not expose the privkey over the API.
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.
It could be part of keystore but as keystore isn't scheduled yet it will have to work for now. In future we can migrate it into keystore.
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.
ok
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.
Word, it would be great if we could move the private key out of the config file in a similar fashion 👍
' | ||
|
||
test_expect_success "try connecting node in public network with priv networks" ' | ||
iptb connect [1-4] 0 |
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.
huh, maybe iptb should return failure if one of those doesnt succeed?
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.
they all fail right now and iptb doesn't return anything.
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.
Yeah, Thats probably something I should get around to fixing on iptb. Seems like a 'bug'.
That said, i don't have any issue with your tests, was just remarking on iptb usage.
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.
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.
Largely LGTM 👍, one comment about a test
@@ -578,6 +581,26 @@ func (r *FSRepo) GetStorageUsage() (uint64, error) { | |||
return du, err | |||
} | |||
|
|||
func (r *FSRepo) SwarmKey() ([]byte, error) { |
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.
Word, it would be great if we could move the private key out of the config file in a similar fashion 👍
|
||
test_expect_success "node 3 (pnet 2) swarm is empty" ' | ||
ipfsi 3 swarm peers && | ||
[ $(ipfsi 3 swarm peers | wc -l) -eq 0 ] |
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.
Should this test wait a bit to make sure it really didn't connect?
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.
The iptb connect command waits for the connect call to finish, by then it is either for sure connected or not.
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.
Cool then
79f57de
to
7ac74c1
Compare
|
||
test_init_ipfs | ||
|
||
export LIBP2P_FORCE_PNET=1 |
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'd find very useful that there was a similar option in the ipfs config file. "EnablePrivateNetworks"
kind of thing. Also, an option to indicate the path to the key?
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.
Agreed with this, but i'm wanting to avoid using config values for features that are currently experimental. Once private networks are more well defined, we can make it an official feature and give it config values to use
@Kubuxu what is the status on the review of the private net crypto code? |
@jbenet told me that he will review it tomorrow, it was 3 days ago. |
e085c67
to
5b4f4e1
Compare
License: MIT Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT Signed-off-by: Jakub Sztandera <[email protected]>
@Kubuxu can we close this now that the other PR merged? |
Yes |
Resolves #3313
Almost done, I still want to create file transfer tests.File transfer tests are down waiting for final CR.