adds metric for turbine retransmit tree mismatch #17351

behzadnouri · 2021-05-20T18:10:21Z

Problem

In order to remove port-based forwarding logic in turbine, we need to first track how often the turbine retransmit/broadcast trees mismatch across nodes. One consistency condition is that if the node is on the critical path (i.e. the first node in each neighborhood), then we expect that the packet arrives at tvu socket as opposed to tvu-forwards.

Summary of Changes

1st commit adds a metric to track how often above consistency condition is not met.
2nd commit just removes the nested for loop. No code logic change.

codecov · 2021-05-20T20:59:36Z

Codecov Report

Merging #17351 (2134646) into master (6cba534) will decrease coverage by 0.0%.
The diff coverage is 92.1%.

@@            Coverage Diff            @@
##           master   #17351     +/-   ##
=========================================
- Coverage    82.7%    82.7%   -0.1%     
=========================================
  Files         421      424      +3     
  Lines      118048   118422    +374     
=========================================
+ Hits        97664    97969    +305     
- Misses      20384    20453     +69

CriesofCarrots

Seems straightforward. Couple nits.

CriesofCarrots · 2021-05-20T21:34:58Z

core/src/retransmit_stage.rs

    let r_lock = r.lock().unwrap();
-    let packets = r_lock.recv_timeout(timer)?;
+    let packets = r_lock.recv_timeout(RECV_TIMEOUT)?;


Suggested change

let packets = r_lock.recv_timeout(RECV_TIMEOUT)?;

let packets = r_lock.recv_timeout(Duration::from_secs(1))?;

Could this just be a one-liner? Or do you expect to use the const elsewhere?

I guess I picked up this habit from previous jobs, to use consts to avoid magic numbers in the code as a means of (mini) documentation and maybe more readability. I guess here it does not matter much either case.

CriesofCarrots · 2021-05-20T21:36:01Z

core/src/retransmit_stage.rs

+        // If the node is on the critical path (i.e. the first node in each
+        // neighborhood), then we expect that the packet arrives at tvu socket
+        // as opposed to tvu-forwards. If this is not the case, then the
+        // turbine broadcast/retransmit tree mismatch across nodes.


Suggested change

// turbine broadcast/retransmit tree mismatch across nodes.

// turbine broadcast/retransmit tree is mismatched across nodes.

?

done, thanks

carllin · 2021-05-20T22:55:43Z

core/src/retransmit_stage.rs

+        // neighborhood), then we expect that the packet arrives at tvu socket
+        // as opposed to tvu-forwards. If this is not the case, then the


This is a clever way to detect this, nice!

In order to remove port-based forwarding logic in turbine, we need to first track how often the turbine retransmit/broadcast trees mismatch across nodes. One consistency condition is that if the node is on the critical path (i.e. the first node in each neighborhood), then we expect that the packet arrives at tvu socket as opposed to tvu-forwards. This commit adds a metric to track how often above condition is not met.

The code can be simplified by just flattening the vector of packets.

Pull request has been modified.

…17392) * adds metric for turbine retransmit tree mismatch In order to remove port-based forwarding logic in turbine, we need to first track how often the turbine retransmit/broadcast trees mismatch across nodes. One consistency condition is that if the node is on the critical path (i.e. the first node in each neighborhood), then we expect that the packet arrives at tvu socket as opposed to tvu-forwards. This commit adds a metric to track how often above condition is not met. (cherry picked from commit 71de021) * removes the nested for loop from retransmit-stage The code can be simplified by just flattening the vector of packets. (cherry picked from commit ff0e623) Co-authored-by: behzad nouri <behzadnouri@gmail.com>

Turbine retransmit logic is based on which socket it received the packet from (i.e `packet.meta.forward`): https://github.com/solana-labs/solana/blob/708bbcb00/core/src/retransmit_stage.rs#L467-L470 This can leave the cluster vulnerable to spoofing and selective propagation of packets; see solana-labs#6672 solana-labs#7774 This commit identifies if the node is on the "critical path" based on its index in the shuffled cluster. If so, it forwards the packet to both neighbors and children; otherwise, the packet is only forwarded to the children. The metrics added in solana-labs#17351 shows that the number of times the index does not match the port is very rare, and therefore this change should be safe.

Turbine retransmit logic is based on which socket it received the packet from (i.e `packet.meta.forward`): https://github.com/solana-labs/solana/blob/708bbcb00/core/src/retransmit_stage.rs#L467-L470 This can leave the cluster vulnerable to spoofing and selective propagation of packets; see #6672 #7774 This commit identifies if the node is on the "critical path" based on its index in the shuffled cluster. If so, it forwards the packet to both neighbors and children; otherwise, the packet is only forwarded to the children. The metrics added in #17351 shows that the number of times the index does not match the port is very rare, and therefore this change should be safe.

Turbine retransmit logic is based on which socket it received the packet from (i.e `packet.meta.forward`): https://github.com/solana-labs/solana/blob/708bbcb00/core/src/retransmit_stage.rs#L467-L470 This can leave the cluster vulnerable to spoofing and selective propagation of packets; see #6672 #7774 This commit identifies if the node is on the "critical path" based on its index in the shuffled cluster. If so, it forwards the packet to both neighbors and children; otherwise, the packet is only forwarded to the children. The metrics added in #17351 shows that the number of times the index does not match the port is very rare, and therefore this change should be safe. (cherry picked from commit 1618386)

…17973) Turbine retransmit logic is based on which socket it received the packet from (i.e `packet.meta.forward`): https://github.com/solana-labs/solana/blob/708bbcb00/core/src/retransmit_stage.rs#L467-L470 This can leave the cluster vulnerable to spoofing and selective propagation of packets; see #6672 #7774 This commit identifies if the node is on the "critical path" based on its index in the shuffled cluster. If so, it forwards the packet to both neighbors and children; otherwise, the packet is only forwarded to the children. The metrics added in #17351 shows that the number of times the index does not match the port is very rare, and therefore this change should be safe. (cherry picked from commit 1618386) Co-authored-by: behzad nouri <behzadnouri@gmail.com>

behzadnouri force-pushed the turbine-tree-mismatch branch from 125796e to ec4e94f Compare May 20, 2021 18:18

behzadnouri requested review from sakridge, carllin and CriesofCarrots May 20, 2021 21:01

CriesofCarrots previously approved these changes May 20, 2021

View reviewed changes

carllin reviewed May 20, 2021

View reviewed changes

carllin previously approved these changes May 20, 2021

View reviewed changes

behzadnouri added 2 commits May 21, 2021 10:46

removes the nested for loop from retransmit-stage

2134646

The code can be simplified by just flattening the vector of packets.

behzadnouri force-pushed the turbine-tree-mismatch branch from ec4e94f to 2134646 Compare May 21, 2021 14:47

behzadnouri merged commit ff0e623 into solana-labs:master May 21, 2021

behzadnouri deleted the turbine-tree-mismatch branch May 21, 2021 17:10

behzadnouri added the v1.6 label May 21, 2021

mergify bot mentioned this pull request May 21, 2021

adds metric for turbine retransmit tree mismatch (backport #17351) #17392

Merged

behzadnouri mentioned this pull request Jun 4, 2021

removes port-based forwarding logic from turbine retransmit #17716

Merged

brooksprumo mentioned this pull request Aug 23, 2021

backport 19361 v16 #19379

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds metric for turbine retransmit tree mismatch #17351

adds metric for turbine retransmit tree mismatch #17351

behzadnouri commented May 20, 2021

codecov bot commented May 20, 2021 •

edited

Loading

CriesofCarrots left a comment

CriesofCarrots May 20, 2021

behzadnouri May 21, 2021

CriesofCarrots May 20, 2021

behzadnouri May 21, 2021

carllin May 20, 2021

	let packets = r_lock.recv_timeout(RECV_TIMEOUT)?;
	let packets = r_lock.recv_timeout(Duration::from_secs(1))?;

	// turbine broadcast/retransmit tree mismatch across nodes.
	// turbine broadcast/retransmit tree is mismatched across nodes.

		// neighborhood), then we expect that the packet arrives at tvu socket
		// as opposed to tvu-forwards. If this is not the case, then the

adds metric for turbine retransmit tree mismatch #17351

adds metric for turbine retransmit tree mismatch #17351

Conversation

behzadnouri commented May 20, 2021

Problem

Summary of Changes

codecov bot commented May 20, 2021 • edited Loading

Codecov Report

CriesofCarrots left a comment

Choose a reason for hiding this comment

CriesofCarrots May 20, 2021

Choose a reason for hiding this comment

behzadnouri May 21, 2021

Choose a reason for hiding this comment

CriesofCarrots May 20, 2021

Choose a reason for hiding this comment

behzadnouri May 21, 2021

Choose a reason for hiding this comment

carllin May 20, 2021

Choose a reason for hiding this comment

codecov bot commented May 20, 2021 •

edited

Loading