network: non-participating nodes request TX gossip only if ForceFetchTransactions: true #3918

brianolson · 2022-04-25T19:13:56Z

Summary

Save bandwidth by having non-participating non-relay nodes opt-out of TX transaction gossip traffic.

Test Plan

New unit tests.
Manual testing has started local private networks to ensure that the new message-of-interest propagated. Cluster tests were run to check bandwidth usage.

…of-interest

fix postMessagesOfInterestThraed() fix maybeSendMessagesOfInterest lock/unlock lots of debug logging

network/wsNetwork.go

wsNetwork rename 'isParticipating' to 'wantTXGossip' to be more precise

codecov-commenter · 2022-05-04T22:26:06Z

Codecov Report

Merging #3918 (df15614) into master (8f3e557) will increase coverage by 4.75%.
The diff coverage is 78.46%.

@@            Coverage Diff             @@
##           master    #3918      +/-   ##
==========================================
+ Coverage   49.78%   54.54%   +4.75%     
==========================================
  Files         409      390      -19     
  Lines       69145    48541   -20604     
==========================================
- Hits        34426    26477    -7949     
+ Misses      30997    19834   -11163     
+ Partials     3722     2230    -1492

Impacted Files	Coverage Δ
data/account/participationRegistry.go	`78.25% <0.00%> (-1.00%)`	⬇️
data/accountManager.go	`68.42% <0.00%> (+1.75%)`	⬆️
node/node.go	`24.96% <33.33%> (+1.76%)`	⬆️
network/wsPeer.go	`67.67% <40.00%> (-0.39%)`	⬇️
network/wsNetwork.go	`64.98% <94.11%> (+2.18%)`	⬆️
catchup/peerSelector.go	`98.95% <0.00%> (-1.05%)`	⬇️
data/abi/abi_type.go	`87.67% <0.00%> (-0.95%)`	⬇️
compactcert/msgp_gen.go
crypto/compactcert/msgp_gen.go
... and 25 more

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 8f3e557...df15614. Read the comment docs.

network/wsNetwork.go

node/node.go

cce · 2022-05-16T13:39:41Z

@brianolson did you see the race condition?

node/node.go

algorandskiy · 2022-05-16T19:51:16Z

network/wsNetwork.go

+	wantTXGossip := atomic.LoadUint32(&wn.wantTXGossip)
+	if wantTXGossip != wantTXGossipNo {
+		wn.messagesOfInterest[protocol.TxnTag] = true
+	} else {
+		delete(wn.messagesOfInterest, protocol.TxnTag)
+	}


this logic looks like belongs to the caller where wn.messagesOfInterest is updated (added, deleted)

good catch. wound up being able to simplify away a chunk of logic that became redundant during development.

network/wsNetwork_test.go

algorandskiy · 2022-05-16T20:02:11Z

network/wsNetwork_test.go

+	incomingMsgSync.Unlock()
+}
+
+func TestWebsocketNetworkTXMessageOfInterestForceTx(t *testing.T) {


this test and above are quite similar, consider generalizing into a _testWebsocket... function

algorandskiy · 2022-05-16T20:03:15Z

network/wsNetwork_test.go

+
+	netB.OnNetworkAdvance()
+	// TODO: better event driven thing for netB sending new MOI
+	time.Sleep(10 * time.Millisecond)


maybe messagesOfInterestCond.Wait()?

Not quite because postMessagesOfInterestThread() is waiting on that and I want to wait until after that thread has had a chance to run. (But nothing that isn't test code would wait for that thread)

network/wsNetwork.go

node/node.go

cce · 2022-05-20T14:52:58Z

network/wsNetwork.go

-			wn.log.Infof("ws send msgOfInterest: %v", err)
-		}
-	}
+	wn.maybeSendMessagesOfInterest(peer, nil)


why not pass in wn.messagesOfInterestEnc here?

because wn.messagesOfInterestEnc should only be grabbed inside wn.messagesOfInterestMu.Lock().
Having it as an argument is really just an optimization for postMessagesOfInterestThread() which already holds the lock while doing a Cond.Wait().
I could put a lock/get/unlock three lines in front of the two other calls to maybeSendMOI() but it's slightly fewer lines of code to do it inside that? (and still reasonable separation of concerns?)

algorandskiy

The code looks fine to me, the tests with time.Sleep could be improved since it adds potential random failure point

network/wsNetwork.go

This reverts commit a74255a.

brianolson · 2022-05-23T16:10:31Z

I think this PR is still generally ready to go except for cce's tweak dismissing the review?

cce · 2022-05-23T18:29:17Z

I noticed an unpredictable race detector error that went away after rerunning, which I guess means it is a flaky race issue:
error case: https://app.circleci.com/pipelines/github/algorand/go-algorand/6649/workflows/cbf48a17-5877-4409-82a1-e4cab32caae3/jobs/115645
successful rerun on the same commit https://app.circleci.com/pipelines/github/algorand/go-algorand/6649/workflows/7812f37e-038f-4dc6-8145-03a371ba3e64

brianolson · 2022-05-23T19:34:31Z

The 'race' is that wsPeer.messagesOfInterestGeneration is written by wsPeer.go code and read by wsNetwork.go code.
I don't think this is really a race, but the race detector notes that an address is written by one thread and read by another and without a lock or an atomic. Adding an atomic store/load would be the fastest, but I'm kinda grumpy because I think this is a false alarm from the race detector.

algorandskiy · 2022-05-23T19:39:21Z

Why? the variable accessed from two different go-routines without synchronization, and this is a race, isn't it?

brianolson · 2022-05-24T15:04:28Z

currently working on a sometimes-deadlock in TestWebsocketNetworkMessageOfInterest()
production code is fine, but the test code poking into wsNetwork has a race...

…m that needed some restructuring

cce · 2022-06-09T01:45:51Z

network/wsNetwork.go

@@ -1677,6 +1725,17 @@ func (wn *WebsocketNetwork) OnNetworkAdvance() {
 	wn.lastNetworkAdvanceMu.Lock()
 	defer wn.lastNetworkAdvanceMu.Unlock()
 	wn.lastNetworkAdvance = time.Now().UTC()
+	if wn.nodeInfo != nil && !wn.relayMessages && !wn.config.ForceFetchTransactions {
+		// if we're not a relay, and not participating, we don't need txn pool
+		wantTXGossip := wn.nodeInfo.IsParticipating()


Looking deeper at OnNetworkAdvance, here it calls nodeInfo.IsParticipating() which contains a call to Ledger.Latest() (which takes a lock on blockQueue.mu.Lock()) and accountManager.HasLiveKeys() (which takes a lock on manager.mu.Lock()).

Before this PR, OnNetworkAdvance used to be very fast — just updating a timestamp. Now it could get stuck for a long time, I know that accountManager.DeleteOldKeys() (which also takes the manager.mu.Lock()) could take seconds in some cases.

Is it possible that OnNetworkAdvance could take too long, and what would be the impact if it did? Agreement synchronously waits for OnNetworkAdvance to finish at the end of every round, when calling the node's EnsureBlock/EnsureValidatedBlock methods. This means the performance of OnNetworkAdvance will delay the start of agreement beginning the next round.

brianolson added 3 commits April 21, 2022 08:38

non-relay non-participating nodes opt out of TX gossip using message-…

98a4650

…of-interest

fix isParticipating logic

643c519

fix postMessagesOfInterestThraed() fix maybeSendMessagesOfInterest lock/unlock lots of debug logging

bug hunting logging

b7442ce

cce reviewed Apr 26, 2022

View reviewed changes

network/wsNetwork.go Outdated Show resolved Hide resolved

brianolson added 2 commits April 26, 2022 16:28

also send MOI on outgoing connections

e9f6d3f

use ForceFetchTransactions config

bb32d57

brianolson added the Enhancement label Apr 27, 2022

brianolson added 2 commits April 28, 2022 10:55

more efficient HasLiveKeys

64a250e

wsNetwork rename 'isParticipating' to 'wantTXGossip' to be more precise

testing

a486325

cce changed the title ~~NPN no TX~~ network: NPN no TX May 4, 2022

brianolson added 2 commits May 4, 2022 17:04

fix TestSlowPeerDisconnection

4a54c54

lint. test tweak

78fdf13

brianolson added the Team Carbon-11 label May 5, 2022

brianolson self-assigned this May 5, 2022

brianolson marked this pull request as ready for review May 5, 2022 00:39

brianolson requested a review from a user May 5, 2022 10:54

cleanup development logging

69ca188

cce reviewed May 9, 2022

View reviewed changes

network/wsNetwork.go Outdated Show resolved Hide resolved

spelling

a02b044

cce reviewed May 13, 2022

View reviewed changes

network/wsNetwork.go Outdated Show resolved Hide resolved

cce reviewed May 13, 2022

View reviewed changes

node/node.go Outdated Show resolved Hide resolved

brianolson added 2 commits May 13, 2022 20:33

review tweak

4f33b10

Merge remote-tracking branch 'origin/master' into npn-no-TX

d09cfab

brianolson added 2 commits May 16, 2022 13:55

use a lock, fix a test race

be6b13e

use a lock, fix a test race

8284573

algorandskiy reviewed May 16, 2022

View reviewed changes

brianolson added 2 commits May 16, 2022 18:18

simplify

2050737

more test conditions. cleanup.

0fe106f

wn.node -> wn.nodeInfo

cbdb6e9

algorandskiy reviewed May 17, 2022

View reviewed changes

network/wsNetwork.go Outdated Show resolved Hide resolved

node/node.go Show resolved Hide resolved

review tweaks

97b1d5c

cce requested review from winder and zeldovich May 20, 2022 14:25

cce reviewed May 20, 2022

View reviewed changes

algorandskiy previously approved these changes May 20, 2022

View reviewed changes

cce reviewed May 20, 2022

View reviewed changes

network/wsNetwork.go Outdated Show resolved Hide resolved

cce reviewed May 20, 2022

View reviewed changes

network/wsNetwork.go Show resolved Hide resolved

cce changed the title ~~network: NPN no TX~~ network: non-participating nodes request TX gossip only if ForceFetchTransactions: true May 20, 2022

algobarb self-requested a review May 20, 2022 17:30

check wn.nodeInfo != nil in OnNetworkAdvance

1ea7803

cce dismissed algorandskiy’s stale review via 1ea7803 May 20, 2022 19:56

cce and others added 2 commits May 20, 2022 15:57

don't set nodeNopeInfo by default

a74255a

Revert "don't set nodeNopeInfo by default"

c2806d7

This reverts commit a74255a.

use atomic to access peer.messagesOfInterestGeneration

4a96326

TestWebsocketNetworkMessageOfInterest had a rare race/deadlock proble…

df15614

…m that needed some restructuring

algorandskiy approved these changes May 25, 2022

View reviewed changes

algorandskiy merged commit 3c4c8fb into algorand:master May 25, 2022

This was referenced May 27, 2022

[Not Merged] go-algorand 3.7.0-beta Release PR #4049

Closed

Release: go-algorand 3.7.0-beta Release PR #4050

Merged

Algo-devops-service mentioned this pull request Jun 3, 2022

go-algorand 3.7.0-stable Release PR #4071

Merged

onetechnical mentioned this pull request Jun 8, 2022

go-algorand 3.7.1-stable Release PR #4091

Merged

cce reviewed Jun 9, 2022

View reviewed changes

onetechnical mentioned this pull request Jun 10, 2022

go-algorand 3.7.2-stable Release PR #4118

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

network: non-participating nodes request TX gossip only if ForceFetchTransactions: true #3918

network: non-participating nodes request TX gossip only if ForceFetchTransactions: true #3918

brianolson commented Apr 25, 2022 •

edited

Loading

codecov-commenter commented May 4, 2022 •

edited by codecov bot

Loading

cce commented May 16, 2022

algorandskiy May 16, 2022

brianolson May 16, 2022

algorandskiy May 16, 2022

algorandskiy May 16, 2022

brianolson May 16, 2022

cce May 20, 2022

brianolson May 23, 2022

algorandskiy left a comment

brianolson commented May 23, 2022

cce commented May 23, 2022

brianolson commented May 23, 2022

algorandskiy commented May 23, 2022

brianolson commented May 24, 2022

cce Jun 9, 2022 •

edited

Loading

network: non-participating nodes request TX gossip only if ForceFetchTransactions: true #3918

network: non-participating nodes request TX gossip only if ForceFetchTransactions: true #3918

Conversation

brianolson commented Apr 25, 2022 • edited Loading

Summary

Test Plan

codecov-commenter commented May 4, 2022 • edited by codecov bot Loading

Codecov Report

cce commented May 16, 2022

algorandskiy May 16, 2022

Choose a reason for hiding this comment

brianolson May 16, 2022

Choose a reason for hiding this comment

algorandskiy May 16, 2022

Choose a reason for hiding this comment

algorandskiy May 16, 2022

Choose a reason for hiding this comment

brianolson May 16, 2022

Choose a reason for hiding this comment

cce May 20, 2022

Choose a reason for hiding this comment

brianolson May 23, 2022

Choose a reason for hiding this comment

algorandskiy left a comment

Choose a reason for hiding this comment

brianolson commented May 23, 2022

cce commented May 23, 2022

brianolson commented May 23, 2022

algorandskiy commented May 23, 2022

brianolson commented May 24, 2022

cce Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

brianolson commented Apr 25, 2022 •

edited

Loading

codecov-commenter commented May 4, 2022 •

edited by codecov bot

Loading

cce Jun 9, 2022 •

edited

Loading