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

whisperv5: integrate whisper and implement API #14540

Merged
merged 11 commits into from
Jun 26, 2017

Conversation

bas-vk
Copy link
Member

@bas-vk bas-vk commented May 29, 2017

This PR holds several things:

  1. it integrates whisper in the node. That means that whisper can be configured through command line flags or through a config file. It can be started by given one or more of the whisper flags. E.g. geth --shh.maxmessagesize <...> would enable whisper.
Whisper (EXPERIMENTAL) OPTIONS:
  --shh                       Enable Whisper
  --shh.maxmessagesize value  Max message size accepted (default: 1048576)
  --shh.pow value             Minimum POW accepted (default: 0.2)
  1. implements the whisper v5 API.

  2. adds the whisper/whisperv5/client package. This client works in a similar way as the ethclient package and can be used to build whisper go DApp's.

@bas-vk bas-vk added the whisper label May 29, 2017
@bas-vk bas-vk force-pushed the whisper-api branch 11 times, most recently from 65aa33d to 7f00d42 Compare June 5, 2017 10:05
@bas-vk bas-vk force-pushed the whisper-api branch 3 times, most recently from 9371b12 to 86dfaa8 Compare June 9, 2017 08:19
@bas-vk bas-vk added the review label Jun 9, 2017
@gluk256
Copy link
Contributor

gluk256 commented Jun 9, 2017

👍

Copy link
Contributor

@gluk256 gluk256 left a comment

Choose a reason for hiding this comment

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

LGTM

w.statsMu.Lock()
defer w.statsMu.Unlock()

return w.stats
Copy link
Contributor

Choose a reason for hiding this comment

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

The locking here seems unnecessary, you're returning the w.stats object, so if the caller does modifications on it, these locks won't prevent that.

Copy link
Member Author

Choose a reason for hiding this comment

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

w.stats is returned by value which makes a copy.

@bas-vk
Copy link
Member Author

bas-vk commented Jun 15, 2017

Last commit removed commented out code.

Copy link
Contributor

@fjl fjl left a comment

Choose a reason for hiding this comment

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

Needs minor code changes.

// You should have received a copy of the GNU Lesser General Public License
// along with the go-ethereum library. If not, see <http://www.gnu.org/licenses/>.

package whisperv5
Copy link
Contributor

Choose a reason for hiding this comment

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

The client should be in its own package, "whisper/shhclient" to remove the rpc dependency from whisper core.

}

Flags = []cli.Flag{WhisperEnabledFlag, MaxMessageSizeFlag, MinPOWFlag}
)
Copy link
Contributor

@fjl fjl Jun 21, 2017

Choose a reason for hiding this comment

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

I don't like adding CLI flags to library packages. We have some in internal/debug, but it should usually be avoided.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can put them into "whisper/shhapi" for now.

@@ -62,7 +62,7 @@ func (e *Envelope) rlpWithoutNonce() []byte {

// NewEnvelope wraps a Whisper message with expiration and destination data
// included into an envelope for network forwarding.
func NewEnvelope(ttl uint32, topic TopicType, aesNonce []byte, msg *SentMessage) *Envelope {
func NewEnvelope(ttl uint32, topic TopicType, aesNonce []byte, msg *sentMessage) *Envelope {
Copy link
Contributor

Choose a reason for hiding this comment

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

Making sentMessage private means that no code outside package whisperv5 can call NewEnvelope. Not sure if that's good or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

You can still create a sent message with whisperv5.NewSentMessage.

"gopkg.in/urfave/cli.v1"
)

//go:generate gencodec -type Config -formats toml -out gen_config.go
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need to use gencodec because none of its features (required fields, field type override, etc.) are used. Please remove the generated file and this line.

"github.com/ethereum/go-ethereum/common/hexutil"
)

/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Please delete the file if these tests are no longer meaningful.


mailServer MailServer // MailServer interface
}

// New creates a Whisper client ready to communicate through the Ethereum P2P network.
func New() *Whisper {
func New(cfg *Config) *Whisper {
whisper := &Whisper{
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add

if cfg == nil {
    cfg = DefaultConfig
}

}
m2[idx] = val
s.vault.Store(m2)
}
Copy link
Contributor

@fjl fjl Jun 21, 2017

Choose a reason for hiding this comment

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

I don't get this type. Please explain. It smells like overengineering.
If you really need a fast concurrent map, use golang.org/x/sync/syncmap.

Copy link
Member Author

@bas-vk bas-vk Jun 21, 2017

Choose a reason for hiding this comment

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

With the api it is possible to change several settings at runtime. This requires locks to prevent simultaneous updates and reads. I have replace the custom map with the syncmap as suggested.

}
*t = BytesToTopic(b)
*t = BytesToTopic(data)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use hexutil.UnmarshalFixedText here.

InvalidPublicKeyErr = errors.New("invalid public key")
InvalidSigningPubKey = errors.New("invalid signing public key")
TooLowPoWErr = errors.New("message rejected, PoW too low")
NoTopicsErr = errors.New("missing topic(s)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Error variables should be named ErrXXX.

@fjl
Copy link
Contributor

fjl commented Jun 21, 2017

General feedback about the API: There are lots of key management methods. These can be hard to use correctly. IMHO we should use the normal keystore for asymmetric keys and push responsibility to generate and manage symmetric keys to the user of the API.

@bas-vk
Copy link
Member Author

bas-vk commented Jun 21, 2017

Moving from the current key management to a "normal" keystore should be something that is done in a different PR. This one is already getting huge.

@gluk256 ping.

@gluk256
Copy link
Contributor

gluk256 commented Jun 21, 2017

Please, let us keep focus on the current task. This PR has one very specific purpose: make the v5 working again. Refactoring could be done only after we will be able to build the project and call all the functions.

@gluk256
Copy link
Contributor

gluk256 commented Jun 21, 2017

@fjl If you want, we can discuss the key management later, and refactor if necessity arise. But as long as whisper remains broken, we should concentrate on fixing it ASAP.

@frozeman
Copy link
Contributor

frozeman commented Jun 22, 2017

Some small fixes necessary after testing:

  • shh_info: message needs renaming to messages in the returned object
  • sometimes generated sym keys don't get stored or something, but creating filters fail with unknown symkey key (even tho they were created before), and shh_hasSymKey returns undefined (should at least return false)
  • i also get errors like specify either a symetric or a asymmetric key when creating filters, even tho i have generated an specified a symKeyID and shh.hasSymKey(id) returns true

@bas-vk
Copy link
Member Author

bas-vk commented Jun 23, 2017

  • shh_info: message needs renaming to messages in the returned object

Renamed

  • sometimes generated sym keys don't get stored or something, but creating filters fail with unknown symkey key (even tho they were created before), and shh_hasSymKey returns undefined (should at least return false)

The only possible return value of shh_hasSymKey is a bool. Would be nice get an example script that reproduces the issue.

  • i also get errors like specify either a symetric or a asymmetric key when creating filters, even tho i have generated an specified a symKeyID and shh.hasSymKey(id) returns true

I expect that you use an invalid field name but an example script would help. Here is an example with symmetric keys that uses filters.

@frozeman
Copy link
Contributor

The last 2 errors were a problem on my side. From what i tested it looks like working. 👍

@frozeman
Copy link
Contributor

Once this is merged, we can add it to the geth console internally: web3/web3.js#902
The files in this PR in the distilled folder can be used.

@frozeman
Copy link
Contributor

Those files can be used: Please include them and test the API and basic functions inside the console.
https://github.com/ethereum/web3.js/tree/develop/dist

@@ -188,17 +188,18 @@ var AppHelpFlagGroups = []flagGroup{
}, debug.Flags...),
},
{
Name: "Whisper (EXPERIMENTAL)",
Copy link
Member

Choose a reason for hiding this comment

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

Please make "Whisper" all uppercase. That's how everything else is.

@karalabe karalabe merged commit feb2932 into ethereum:master Jun 26, 2017
@karalabe karalabe added this to the 1.6.7 milestone Jun 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants