Transaction gossiping by tzdybal · Pull Request #29 · evstack/ev-node

tzdybal · 2021-03-22T11:39:47Z

Transaction gossiping, using

lazyledger-core/tendermint mempool
libp2p GossipSub on P2P layer

Resolves #3.
Depends on #17 and #30.

codecov-io · 2021-03-23T14:10:10Z

Codecov Report

Merging #29 (aff4203) into main (2ec85bb) will decrease coverage by 30.79%.
The diff coverage is 33.33%.

@@             Coverage Diff             @@
##             main      #29       +/-   ##
===========================================
- Coverage   71.81%   41.02%   -30.80%     
===========================================
  Files           3        4        +1     
  Lines         110      234      +124     
===========================================
+ Hits           79       96       +17     
- Misses         16      121      +105     
- Partials       15       17        +2

Impacted Files	Coverage Δ
rpcclient/local.go	`6.25% <0.00%> (ø)`
p2p/client.go	`70.75% <65.00%> (+1.60%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ec85bb...aff4203. Read the comment docs.

docs/lazy-adr/adr-002-mempool.md

liamsi · 2021-03-23T15:13:28Z

docs/lazy-adr/adr-002-mempool.md

+  * error prone
+* Re-using other mempool (Celo, Prysm, etc)
+  * different API
+  * potential licensing issues


We should still list cons even, or especially, for the decided for approach (of using the tm mempool). Cons I can think of:

inherit all limitations of the tendermint mempool (no prioritization of Txs - maybe list some issues in TM/TM-specs)

legacy code base (the tendermint mempool exists for a while now)

...

p2p/client_test.go

liamsi · 2021-03-24T14:57:54Z

p2p/client_test.go

+	time.Sleep(5 * time.Second)
+
+	// gossip from client 4
+	err := clients[4].GossipTx(ctx, []byte("sample tx"))


So the problem seems to be that the message we are trying to publish does not get validated properly: https://github.com/libp2p/go-libp2p-pubsub/blob/5457a2845b06d859df6d3683867808e46a15c500/validation.go#L242-L247

liamsi · 2021-03-25T12:07:56Z

p2p/client_test.go

 	assert.NoError(err)

-	nCtx, nCancel := context.WithTimeout(ctx, 1*time.Second)
+	nCtx, nCancel := context.WithTimeout(ctx, 300*time.Second)


Wowser. Does that mean, the messages gets now written to the channel but the client isn't able to retrieve it now?

This is a work in progress, big value to give me more time in debugger ;)

Doesn't a breakpoint stop the "world"? Or does that depend on which debugger you are using?

liamsi · 2021-03-25T13:21:24Z

p2p/client.go

-const reAdvertisePeriod = 1 * time.Hour
+const (
+	// reAdvertisePeriod defines a period after which P2P client re-attempt advertising namespace in DHT.
+	reAdvertisePeriod = 1 * time.Hour


That is so long because once they've advertized this it won't change?

liamsi · 2021-03-25T14:00:59Z

It seems the nodes are not really connected with each other now.

I expect this to work, but it doesn't.

Number 003 is selected, because 002 is proposed in #29.

liamsi · 2021-03-29T12:29:29Z

node/node.go

 }

-func NewNode(ctx context.Context, conf config.NodeConfig, nodeKey crypto.PrivKey, clientCreator proxy.ClientCreator, logger log.Logger) (*Node, error) {
+func NewNode(ctx context.Context, conf config.NodeConfig, nodeKey crypto.PrivKey, clientCreator proxy.ClientCreator, genesis *types.GenesisDoc, logger log.Logger) (*Node, error) {


optional nit: IMO a line break per parameter would increase readability.

liamsi

LGMT 👍🏼

liamsi · 2021-03-29T12:34:28Z

p2p/client.go

+type Tx struct {
+	Data []byte
+}


IMO, we will most likely keep it that way or even make it an alias like in tendermint: https://github.com/tendermint/tendermint/blob/9ecfcc93a6364bbecc9e1d7740b53602eda667eb/types/tx.go#L14-L17

Reason is the abstraction between tendermint (and with it optimint) and the abci app (that understands the Tx).

It is certainly not the right place for the type definition though but that will be part of that refactoring mentioned in the todo I guess.

liamsi · 2021-03-29T12:39:51Z

p2p/client_test.go

+
+	// wait for clients that should receive Tx
+	wg.Wait()
+}


👍🏼 Nice job on the test.

Wondertan · 2021-03-29T12:32:16Z

p2p/client.go

+	select {
+	case <-ctx.Done():
+		return ctx.Err()
+	case <-c.dht.RefreshRoutingTable():
+	}


Wondertan · 2021-03-29T12:56:39Z

p2p/client_test.go

+	// TODO(tzdybal): is there a better way to wait for readiness?
+	time.Sleep(1 * time.Second)


Nope) Even tests in libp2p use sleep but with much less time) Though I've seen discussions about fixing this several time thought more than a year)

1s is empirical result for GitHub actions ;) Locally even 100ms works.

tzdybal force-pushed the tzdybal/p2p/discovery branch from e97f4bf to 7777439 Compare March 23, 2021 09:27

Base automatically changed from tzdybal/p2p/discovery to main March 23, 2021 09:34

tzdybal force-pushed the tzdybal/p2p/tx_gossip branch from 5cf26e4 to 635cdb7 Compare March 23, 2021 10:23

tzdybal changed the base branch from main to tzdybal/p2p/refactor_tests March 23, 2021 10:24

tzdybal force-pushed the tzdybal/p2p/tx_gossip branch from 1cb15f9 to 1fb6bd4 Compare March 23, 2021 13:09

tzdybal changed the base branch from tzdybal/p2p/refactor_tests to main March 23, 2021 13:09

tzdybal force-pushed the tzdybal/p2p/tx_gossip branch from 1fb6bd4 to cc64246 Compare March 23, 2021 14:08

liamsi reviewed Mar 23, 2021

View reviewed changes

docs/lazy-adr/adr-002-mempool.md Show resolved Hide resolved

liamsi reviewed Mar 23, 2021

View reviewed changes

docs/lazy-adr/adr-002-mempool.md Show resolved Hide resolved

liamsi reviewed Mar 23, 2021

View reviewed changes

tzdybal force-pushed the tzdybal/p2p/tx_gossip branch 2 times, most recently from db9d945 to aff4203 Compare March 24, 2021 10:10

tzdybal mentioned this pull request Mar 24, 2021

Pass genesis doc to optimint node celestiaorg/cosmos-sdk#24

Merged

1 task

liamsi reviewed Mar 24, 2021

View reviewed changes

p2p/client_test.go Outdated Show resolved Hide resolved

liamsi reviewed Mar 24, 2021

View reviewed changes

p2p/client_test.go Outdated Show resolved Hide resolved

liamsi reviewed Mar 24, 2021

View reviewed changes

liamsi reviewed Mar 25, 2021

View reviewed changes

tzdybal added 10 commits March 27, 2021 11:45

Transaction gossiping: first draft of p2p layer

200e067

Add lazyledger-core/tendermint mempool

f45be18

Add first, simplistic test of local RPC client

4ee10fd

Add ADR-002 - Mempool

26f64c7

Pass ChainID from genesis to P2P client

d685fcf

Test of gossiping (broken)

8668313

I expect this to work, but it doesn't.

Enable usage of not-bogus keys, handle chainID in tests

07dc341

Debugging version

03679eb

Dirty, debug code, but at least it gossips

eb0825c

Cleanup in client

e331aa3

tzdybal force-pushed the tzdybal/p2p/tx_gossip branch 3 times, most recently from 4f129ce to a0ef894 Compare March 27, 2021 11:48

Cleanup in tests

4683436

tzdybal force-pushed the tzdybal/p2p/tx_gossip branch from a0ef894 to 4683436 Compare March 27, 2021 11:54

tzdybal marked this pull request as ready for review March 29, 2021 08:58

tzdybal added a commit that referenced this pull request Mar 29, 2021

Describe peer discovery in ADR

96274e7

Number 003 is selected, because 002 is proposed in #29.

tzdybal mentioned this pull request Mar 29, 2021

Describe peer discovery in ADR #33

Merged

liamsi reviewed Mar 29, 2021

View reviewed changes

liamsi approved these changes Mar 29, 2021

View reviewed changes

Wondertan approved these changes Mar 29, 2021

View reviewed changes

tzdybal merged commit e553342 into main Mar 30, 2021

tzdybal deleted the tzdybal/p2p/tx_gossip branch May 2, 2023 19:40

claude bot mentioned this pull request Dec 9, 2025

feat: DA Client remove interface part 3, replace types with new code #2910

Merged

		// TODO(tzdybal): is there a better way to wait for readiness?
		time.Sleep(1 * time.Second)

Conversation

tzdybal commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

liamsi Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

liamsi Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liamsi commented Mar 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liamsi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

tzdybal commented Mar 22, 2021 •

edited

Loading

codecov-io commented Mar 23, 2021 •

edited

Loading

liamsi Mar 23, 2021 •

edited

Loading

liamsi Mar 24, 2021 •

edited

Loading