bzzeth: Phase 1 #1685

jmozah · 2019-08-22T10:03:46Z

Phase 1 does the following things

Handles NewBlockHeaders from trinity
Sends GetBlockHeaders and Handles BlockHeaders (which means it stores it too)
Unit test cases

And test this with Trinity client.

jmozah · 2019-09-16T14:03:03Z

@zelig @janos
Even though the testcase passes... there is an issue with decoding "Headers" in BlockHeaders message.
This is because of the rlp.RawValue which is not getting decoded properly.

This is the error i get

ERROR[09-16|17:57:55.915|github.com/ethersphere/swarm/p2p/protocols/protocol.go:238] peer.handleIncoming                      err="Invalid message (RLP error): <= msg #3 (16 bytes): rlp: element is larger than containing list"

But this is not causing a issue in when we test with the Trinity client.
We can either spend time fixing this... OR in phase 2 we are any way going to replace this with proper BlockHeader struct...we can tackle it there...

So i am submitting this for review.

Co-authored-by: zelig <viktor.tron@gmail.com> Co-authored-by: jmozah <zahoor@zahoor.in>

zelig

minor

zelig · 2019-09-19T17:43:35Z

bzzeth/bzzeth.go

 		default:
+			p.logger.Error("Invalid msg")


there is no need for default case, p2p protocols knows about possible message types

zelig · 2019-09-20T10:16:03Z

bzzeth/bzzeth.go

+		vhash := addresses[i]
+		if wantHeaderFunc(vhash, b.kad) {
+			hashes = append(hashes, vhash)
+		} else {


This is super nit picking but call wantedHeader but if not wanted we report not in proximity. so you display more specific property than granted by wantedHeaders which can also be overwritten.
Generally code is simpler to interpret if it avoids these latent logical leaps.

I am not sure i understand this comment. Are you saying not to log.Trace if header is not in proximity?

zelig · 2019-09-20T10:19:45Z

bzzeth/bzzeth.go

+		hdr := make([]byte, len(h))
+		copy(hdr, h)
+		wg.Add(1)
+		go func() {


could we rewrite this using errgroup

in addition, you could probably avoid the copy operation with changing
go func() { to go func(h []byte) { then calling the closure with (h[:])

errGroups Go routine can onlu accept func().. so cant pass it like that..
func (g *Group) Go(f func() error)

ok then shadow it?

zelig · 2019-09-20T10:20:54Z

bzzeth/bzzeth.go

+	req.lock.RLock()
+	defer req.lock.RUnlock()
+	rcvdFlag, ok = req.hashes[addr]
+	return


return explicit values as per our guidelines :)

zelig · 2019-09-20T10:23:55Z

bzzeth/bzzeth_test.go

+		})
+}
+
+func blockHeaderExchange(tester *p2ptest.ProtocolTester, peerID enode.ID, requestID uint32, wantedData []rlp.RawValue) error {


this does not seem to test much

blockHeaderExchange does not test anything from the protocol response perspective, but TestNewBlockHeaders testcase checks the effect of receiving this message. Especially see the functions checkStorage and checkDelivery.

bzzeth/bzzeth.go

bzzeth/bzzeth_test.go

bzzeth/wire.go

acud · 2019-09-24T04:47:03Z

bzzeth/bzzeth.go

+	yes, err := b.netStore.Store.HasMulti(ctx, addresses...)
+	if err != nil {
+		log.Error("Error checking hashesh in store", "Reason", err)
+		return


consider dropping the peer here before returning

acud · 2019-09-24T04:48:01Z

bzzeth/bzzeth.go

+	addresses := make([]chunk.Address, len(*msg))
+	for i, h := range *msg {
+		addresses[i] = h.Hash.Bytes()
+		log.Trace("Received hashes ", "Header", hex.EncodeToString(h.Hash.Bytes()))


use p.logger instead, also in 135, remove the extra space after "hashes " and don't capitalize the words in the logger, we usually log lowercase

acud · 2019-09-24T04:48:50Z

bzzeth/bzzeth.go

+	deliveries := make(chan []byte)
+	req, err := p.getBlockHeaders(ctx, hashes, deliveries)
+	if err != nil {
+		p.logger.Error("Error sending GetBlockHeader message", "Reason", err)


i meant instead of Reason write err

acud · 2019-09-24T06:44:04Z

bzzeth/bzzeth.go

+		return
+	}
+
+	// convert rlp.RawValue to bytes


wrong comment

acud · 2019-09-24T06:48:53Z

bzzeth/bzzeth.go

+	}
+
+	// convert rlp.RawValue to bytes
+	var headers [][]byte


i meant it might need shadowing inside the loop. for now just make the above change with:

headers := make([][]byte, len(msg.Headers)) for i, h ... { headers[i]=h }

acud · 2019-09-24T06:50:05Z

bzzeth/bzzeth.go

+		hdr := make([]byte, len(h))
+		copy(hdr, h)
+		wg.Add(1)
+		go func() {


ok then shadow it?

zelig

i think it is great now

zelig · 2019-09-24T21:26:51Z

bzzeth/bzzeth.go

+	close(errC)
+
+	// Store all the valid header chunks in one shot
+	results, err := b.netStore.Put(ctx, chunk.ModePutUpload, chunks...)


skylenet · 2019-09-25T11:07:55Z

This is failing on Appveyor with:

--- FAIL: TestNewBlockHeaders (0.03s)
    bzzeth_test.go:89: Could not remove localstore dir

It's now also happening on master after being merged.

jmozah self-assigned this Aug 22, 2019

zelig force-pushed the bzz-eth branch from e04406b to 6b2cb2b Compare August 22, 2019 10:23

jmozah force-pushed the bzz-eth-phase1 branch from f44d510 to d6e5751 Compare August 22, 2019 10:27

jmozah mentioned this pull request Aug 22, 2019

[EPIC] Ethereum header chain over swarm ethersphere/user-stories#9

Open

6 tasks

jmozah force-pushed the bzz-eth branch 2 times, most recently from c1035d9 to 37d04da Compare August 28, 2019 12:29

jmozah changed the base branch from bzz-eth to master August 29, 2019 10:14

jmozah force-pushed the bzz-eth-phase1 branch 3 times, most recently from 2fe8cf5 to 511f495 Compare August 29, 2019 17:21

jmozah mentioned this pull request Sep 2, 2019

[User-Story] Bzz-Eth Phase 1 ethersphere/user-stories#54

Closed

zelig added blockchain-on-swarm in progress labels Sep 3, 2019

jmozah force-pushed the bzz-eth-phase1 branch 3 times, most recently from 5bfdbfb to fa28672 Compare September 16, 2019 13:34

jmozah requested review from janos and zelig September 16, 2019 13:58

acud and others added 7 commits September 18, 2019 18:26

bzzeth: phase 0: handshake

6667faa

Co-authored-by: zelig <viktor.tron@gmail.com> Co-authored-by: jmozah <zahoor@zahoor.in>

Added the Phase 1 related stuff from the old branch

fc6f6cf

Added phase 1 support

36ab4f0

Fixed test cases for storage and delivery

3fbf6fc

Fix arguments for Network

2fb1417

Cleanup phase 0 testcase leftovers

4610d4a

Fix linting

5b9c825

jmozah force-pushed the bzz-eth-phase1 branch from fa28672 to 5b9c825 Compare September 18, 2019 12:56

jmozah removed the in progress label Sep 18, 2019

zelig added the ready for review label Sep 19, 2019

zelig suggested changes Sep 20, 2019

View reviewed changes

acud suggested changes Sep 23, 2019

View reviewed changes

Fixed review comments

c458165

acud reviewed Sep 24, 2019

View reviewed changes

zelig approved these changes Sep 24, 2019

View reviewed changes

acud approved these changes Sep 25, 2019

View reviewed changes

acud merged commit d0bf0b6 into master Sep 25, 2019

skylenet added this to the 0.5.0 milestone Sep 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bzzeth: Phase 1 #1685

bzzeth: Phase 1 #1685

jmozah commented Aug 22, 2019

jmozah commented Sep 16, 2019

zelig left a comment

zelig Sep 19, 2019

jmozah Sep 23, 2019

zelig Sep 20, 2019

jmozah Sep 23, 2019

zelig Sep 20, 2019

acud Sep 23, 2019

jmozah Sep 23, 2019

acud Sep 24, 2019

zelig Sep 20, 2019

jmozah Sep 23, 2019

zelig Sep 20, 2019

jmozah Sep 23, 2019

acud Sep 24, 2019

acud Sep 24, 2019

acud Sep 24, 2019

acud Sep 24, 2019

acud Sep 24, 2019

acud Sep 24, 2019

zelig left a comment

zelig Sep 24, 2019

skylenet commented Sep 25, 2019

bzzeth: Phase 1 #1685

bzzeth: Phase 1 #1685

Conversation

jmozah commented Aug 22, 2019

jmozah commented Sep 16, 2019

zelig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zelig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skylenet commented Sep 25, 2019