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

Switch code from whisperV5 to whisperV6 #638

Closed
JekaMas opened this issue Feb 8, 2018 · 12 comments
Closed

Switch code from whisperV5 to whisperV6 #638

JekaMas opened this issue Feb 8, 2018 · 12 comments
Assignees

Comments

@JekaMas
Copy link
Contributor

JekaMas commented Feb 8, 2018

Problem

Switch code from whisperV5 to whisperV6

Implementation

Change imports from "github.com/ethereum/go-ethereum/whisper/whisperv5" to whisper "github.com/ethereum/go-ethereum/whisper/whisperv6 in status-go code.
Also make a patch to change imports in Geth packages: go-ethereum\whisper\notifications and go-ethereum\whisper\mailserver.

Acceptance Criteria

All tests are passed. Patch for go-ethereum packages added.

Part of #634

@JekaMas
Copy link
Contributor Author

JekaMas commented Feb 12, 2018

@pombeirp Please, also take attension to switching Status node in 'light' whisper mode. WhisperV6 introduces 2 node types: full and light (see https://github.com/ethereum/go-ethereum/blob/master/whisper/whisperv6/whisper.go#L1034). Full node receives all messages. Light node shares its bloom filter and expects to receive only matched messages.
As far as i understand we need offline mail server to be a full node and status node to be a light node. Otherwice user couldnt add its bloom filter (https://github.com/ethereum/go-ethereum/blob/master/whisper/whisperv6/whisper.go#L551 https://github.com/ethereum/go-ethereum/blob/master/whisper/whisperv6/peer.go#L234 )

GitHub
go-ethereum - Official Go implementation of the Ethereum protocol
GitHub
go-ethereum - Official Go implementation of the Ethereum protocol
GitHub
go-ethereum - Official Go implementation of the Ethereum protocol

@pedropombeiro
Copy link
Contributor

@JekaMas I also noticed a few things I'll need to investigate further:

  • NewSentMessage has been made private in Whisper v6, but we're using them, so I'll need to see if I'll make a public version of it.
  • EnvelopeVersion no longer exists in Whisper v6, and they no longer use deriveKeyMaterial, which made use of it previously:
	// double check is necessary, because deriveKeyMaterial() is very slow
	if whisper.symKeys[id] != nil {
		return "", fmt.Errorf("critical error: failed to generate unique ID")
	}

@JekaMas
Copy link
Contributor Author

JekaMas commented Feb 12, 2018

@pombeirp 1. https://github.com/ethereum/go-ethereum/blob/master/whisper/whisperv6/message.go#L92 NewSentMessage is public in current V6 version.

GitHub
go-ethereum - Official Go implementation of the Ethereum protocol

@JekaMas
Copy link
Contributor Author

JekaMas commented Feb 12, 2018

  1. Could we make something like:
// call
derived, err := deriveKeyMaterial(key, whisper.ProtocolVersion)
...

// deriveKeyMaterial derives symmetric key material from the key or password./~~~
// pbkdf2 is used for security, in case people use password instead of randomly generated keys.
func deriveKeyMaterial(key []byte, version uint64) (derivedKey []byte, err error) {
	if version <= 6  {
		// kdf should run no less than 0.1 seconds on average compute,
		// because it's a once in a session experience
		derivedKey := pbkdf2.Key(key, nil, 65356, 32, sha256.New)
		return derivedKey, nil
	} else {
		return nil, errors.New("unknown version")
	}
}

It looks like it doesn't make big difference about versions. It just checks is current whisper's version is known to notification. So lets make this fix and create issue to check do we need notification package at all.

@pedropombeiro
Copy link
Contributor

pedropombeiro commented Feb 12, 2018

@pombeirp 1. https://github.com/ethereum/go-ethereum/blob/master/whisper/whisperv6/message.go#L92 NewSentMessage is public in current V6 version.

Hah, NewSentMessage was changed yesterday! Will import that commit then.

@pedropombeiro
Copy link
Contributor

pedropombeiro commented Feb 12, 2018

@JekaMas regarding full/light node, that means that for the full node we'll set nil bloom filter and for the light node we'll set a bloom filter with another value that doesn't start with 0xFF? I haven't yet read anything about the usage of the bloom filters in geth (just the theory).

@JekaMas
Copy link
Contributor Author

JekaMas commented Feb 12, 2018

@pombeirp Lets check Ethereum`s code:

func isFullNode(bloom []byte) bool {
	if bloom == nil {
		return true
	}
	for _, b := range bloom {
		if b != 255 {
			return false
		}
	}
	return true
}

It looks like any 0xFF byte makes node full.

It test branch with whisperV6 i've solved this with hack in Subscribe. I changed:

func (whisper *Whisper) Subscribe(f *Filter) (string, error) {
	s, err := whisper.filters.Install(f)
	if err == nil {
		whisper.updateBloomFilter(f)
	}
	return s, err
}

To

func (whisper *Whisper) Subscribe(f *Filter) (string, error) {
	s, err := whisper.filters.Install(f)
	if err == nil {
		if whisper.BloomFilter() == nil {
			whisper.newBloomFilter()
		}
		whisper.updateBloomFilter(f)
	}
	return s, err
}

@dshulyak
Copy link
Contributor

It looks like any 0xFF byte makes node full.

Maybe thats a typo, but node is full only if all of the 64 bytes are 0xFF. There is even a unit test for that.

Why do you need that hack? Node with full filter will always stay full irrelevant of updates.

        full := make([]byte, 64)
	notFull := make([]byte, 64)
	for i := 0; i < 64; i++ {
		full[i] = 255
		notFull[i] = 254
	}
	aggregate := addBloom(full, notFull)
	isFullNode(aggregate) // always TRUE

@JekaMas
Copy link
Contributor Author

JekaMas commented Feb 13, 2018

I dont want any hacks at all :) I just shown the result state we want.

@pedropombeiro
Copy link
Contributor

In order to fully test this (especially the bloom filter initialization), we need to complete #640

@pedropombeiro
Copy link
Contributor

This issue is ending up being addressed by #665

pedropombeiro pushed a commit that referenced this issue Feb 26, 2018
pedropombeiro pushed a commit that referenced this issue Feb 26, 2018
pedropombeiro pushed a commit that referenced this issue Feb 26, 2018
pedropombeiro pushed a commit that referenced this issue Feb 26, 2018
pedropombeiro pushed a commit that referenced this issue Feb 26, 2018
… 1.8.1 vendor files. Part of #638"

This reverts commit 6aefb4c.
pedropombeiro pushed a commit that referenced this issue Feb 26, 2018
… 1.8.1 vendor files. Part of #638"

This reverts commit 6aefb4c.
pedropombeiro pushed a commit that referenced this issue Feb 27, 2018
… 1.8.1 vendor files. Part of #638"

This reverts commit 6aefb4c.
pedropombeiro pushed a commit that referenced this issue Feb 27, 2018
* Update `github.com/ethereum/go-ethereum` package to 1.8.1 branch. Part of #638
* Fix code due to some signature changes. Part of #638
* use upstream for whisper backend
* Add patch to downgrade usage of Whisper v6 to v5 in some geth 1.8.1 vendor files. Part of #638
* Take into account the DNS rebinding protection introduced in 1.8.0 by adding exception for localhost. Part of #638
* Add patches required for cross-compiled builds starting with geth 1.8.0. Only applied during build. Part of #638
* Update expected JSON result in `TestRegressionGetTransactionReceipt()` and `TestCallRawResultGetTransactionReceipt()`. Part of #665
* Fix some failing e2e tests. Part of #638
* Address comments in PR #702. Part of #638
pedropombeiro pushed a commit that referenced this issue Feb 27, 2018
… 1.8.1 vendor files. Part of #638"

This reverts commit 6aefb4c.
pedropombeiro pushed a commit that referenced this issue Feb 27, 2018
… 1.8.1 vendor files. Part of #638"

This reverts commit 6aefb4c.
@pedropombeiro pedropombeiro reopened this Feb 27, 2018
pedropombeiro pushed a commit that referenced this issue Mar 1, 2018
… 1.8.1 vendor files. Part of #638"

This reverts commit 6aefb4c.
pedropombeiro pushed a commit that referenced this issue Mar 2, 2018
* Update project to use Whisper v6. Part of #638

* Revert "Add patch to downgrade usage of Whisper v6 to v5 in some geth 1.8.1 vendor files. Part of #665" - this reverts commit 6aefb4c.

* Enable light mode on Whisper v6 for non-mail servers. Part of #638

* Fix race condition in whisperv6/peer.go. Part of #665 (PR already accepted upstream for 1.8.2)

* Update bootnode addresses in staticnodes.json. Part of #638

* Add `shh.lightclient` flag and tests for bloom filter setting logic. Part of #638

* Move MakeTestNodeConfig to utils. Part of #638

* Reduce PoW in `whisper_jail_test.go` to fix flaky test. Part of #638
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

No branches or pull requests

3 participants