Allow for out of order nonce #2710

cyberj0g · 2023-01-05T11:51:18Z

Implementation of #2661 to fix #2614 based on introducing sorted hash map to store seen nonce values. Also added @yondonfu's test from corresponding branch, which now passes.

thomshutt · 2023-01-05T14:40:05Z

pm/recipient_test.go

@@ -224,8 +225,8 @@ func TestReceiveTicket_ValidNonWinningTicket(t *testing.T) {
 	recipientRand := genRecipientRand(sender, secret, params)
 	senderNonce := r.(*recipient).senderNonces[recipientRand.String()]

-	if senderNonce.nonce != newSenderNonce {
-		t.Errorf("expected senderNonce to be %d, got %d", newSenderNonce, senderNonce.nonce)
+	if senderNonce.Front().Key != newSenderNonce {


This is probably me misunderstanding, but I'd expect this be .Back() to look at the most recent one

Right, Back() would be most recent nonce, but in this test we test only one ticket anyway.

thomshutt

LGTM, probably good to wait for Yondon's approval too since he originally documented this issue

yondonfu · 2023-01-06T15:08:25Z

pm/recipient.go

+	r.senderNonces[randStr].Set(ticket.SenderNonce, ticket.ParamsExpirationBlock)
+	// delete first seen nonce, if ordered map exceeds max size
+	if r.senderNonces[randStr].Len() > maxSenderNonces {
+		r.senderNonces[randStr].Delete(r.senderNonces[randStr].Front().Key)


Seems simpler to just return an error if the max size is hit. If we delete the first seen nonce, then that means that in theory B could re-use that deleted nonce after the max size is hit and if we return an error if the max size is hit we at least wouldn't need to reason about this scenario. This would mean that once the max size is hit, B would need a new set of ticket params with a fresh recipientRandHash, but as mentioned in the original issue at the moment this should happen after B receives a response for each transcoded segment.

If we don't need to delete the first seen senderNonce value I think that would also simplify the data struct impl s.t. we might not need to depend on the custom ordered map package.

WDYT?

yondonfu · 2023-01-06T15:12:10Z

pm/recipient.go

+		r.senderNonces[randStr] = orderedmap.NewOrderedMap()
+	}
+	// add new nonce
+	r.senderNonces[randStr].Set(ticket.SenderNonce, ticket.ParamsExpirationBlock)


I don't think we need to track an expiration block per senderNonce - we only need to track an expiration block per recipientRandHash (aka randStr in this function). And we only need to track seen senderNonce values associated with a particular recipientRandHash until that recipientRandHash expires.

I think this would simplify the state tracking since we can track an expiration block per recipientRandHash instead of an expiration block for every senderNonce value. With the current impl I think the map ends up looking like { key: 1 value: X, key: 2 value: X, key 3 value: X }.

yondonfu · 2023-01-06T15:14:00Z

pm/recipient.go

-				if sn.expirationBlock.Cmp(latestL1Block) <= 0 {
-					delete(r.senderNonces, recipientRand)
+			for recipientRand, nonceMap := range r.senderNonces {
+				for _, key := range nonceMap.Keys() {


If we track a single expiration block per recipientRandHash then we wouldn't need the nested loop here and instead could just clear all senderNonce values for the recipientRandHash once the expirationBlock is reached.

Quintendos · 2023-01-11T11:59:47Z

Ivan to fix up Yondon's comments and then ready to merge.

cyberj0g · 2023-01-12T08:30:06Z

Thanks for reviews @yondonfu @thomshutt, if we don't want FIFO and don't need to track expiration block number per nonce, that makes the logic simpler and removes sorted hash map dependency. I still used map instead of slice to have O(1) retrieval.

codecov · 2023-01-12T14:09:46Z

Codecov Report

Merging #2710 (d5a364f) into master (a5606ca) will increase coverage by 0.01598%.
The diff coverage is 100.00000%.

❗ Current head d5a364f differs from pull request most recent head c6237ae. Consider uploading reports for the commit c6237ae to get more accurate results

@@                 Coverage Diff                 @@
##              master       #2710         +/-   ##
===================================================
+ Coverage   56.33265%   56.34863%   +0.01598%     
===================================================
  Files             88          88                 
  Lines          19131       19138          +7     
===================================================
+ Hits           10777       10784          +7     
  Misses          7765        7765                 
  Partials         589         589

Impacted Files	Coverage Δ
pm/recipient.go	`89.95633% <100.00000%> (+0.31669%)`	⬆️

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 a5606ca...c6237ae. Read the comment docs.

Impacted Files	Coverage Δ
pm/recipient.go	`89.95633% <100.00000%> (+0.31669%)`	⬆️

yondonfu

Left a few nits. Feel free to address prior to merging.

Other than that LGTM.

yondonfu · 2023-01-12T19:54:43Z

pm/recipient.go

-	sn, ok := r.senderNonces[randStr]
-	if ok && ticket.SenderNonce <= sn.nonce {
-		return errors.Errorf("invalid ticket senderNonce sender=%v nonce=%v highest=%v", ticket.Sender.Hex(), ticket.SenderNonce, sn.nonce)
+	senderNonce, randKeySeen := r.senderNonces[randStr]


Suggested change

senderNonce, randKeySeen := r.senderNonces[randStr]

senderNonces, randKeySeen := r.senderNonces[randStr]

Nit: The plural feels a bit clearer here since the map is tracking a collection of nonces as opposed to a single one.

yondonfu · 2023-01-12T20:02:09Z

pm/recipient.go

@@ -86,7 +89,7 @@ type recipient struct {
 	maxfacevalue *big.Int

 	senderNonces map[string]*struct {
-		nonce           uint32
+		nonceSeen       map[uint32]byte


Nit: Is there an advantage of using byte here instead of a bool?

IIUC a bool should be 1 byte which would be the same size as a byte [1] so I don't think there are any storage benefits of using byte. If this is the case, I suggest using a bool as I think using true/false values will be clearer for code readability than using 0/1.

[1] Can verify using unsafe.Sizeof() on a bool var and a byte var.

pm/recipient.go

cyberj0g requested review from thomshutt and yondonfu January 5, 2023 11:51

thomshutt reviewed Jan 5, 2023

View reviewed changes

thomshutt approved these changes Jan 6, 2023

View reviewed changes

yondonfu reviewed Jan 6, 2023

View reviewed changes

leszko mentioned this pull request Jan 10, 2023

Rewards Claiming Failing with Nonce Too Low in v0.5.36 #2718

Open

yondonfu and others added 3 commits January 12, 2023 11:21

(WIP) test/e2e: Add parallel segment push test for B

521f4c2

Use sorted hash map for nonce accounting

89b86b6

Address review comments - no need for sorted hash map

d3a7d7d

cyberj0g force-pushed the ip/fix-sender-nonce branch from 07b1a3a to d3a7d7d Compare January 12, 2023 08:21

go mod tidy

1f3f313

cyberj0g requested a review from yondonfu January 12, 2023 08:30

go fmt

d5a364f

yondonfu approved these changes Jan 12, 2023

View reviewed changes

address review comments

fc436be

cyberj0g force-pushed the ip/fix-sender-nonce branch from 734de92 to fc436be Compare January 13, 2023 07:19

address review comments

c6237ae

cyberj0g merged commit 11af0a6 into master Jan 13, 2023

cyberj0g deleted the ip/fix-sender-nonce branch January 13, 2023 08:22

yondonfu mentioned this pull request May 31, 2023

pm: Add README #1099

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for out of order nonce #2710

Allow for out of order nonce #2710

cyberj0g commented Jan 5, 2023

thomshutt Jan 5, 2023 •

edited

Loading

cyberj0g Jan 6, 2023

thomshutt left a comment

yondonfu Jan 6, 2023

yondonfu Jan 6, 2023

yondonfu Jan 6, 2023

Quintendos commented Jan 11, 2023

cyberj0g commented Jan 12, 2023

codecov bot commented Jan 12, 2023 •

edited

Loading

yondonfu left a comment •

edited

Loading

yondonfu Jan 12, 2023 •

edited

Loading

yondonfu Jan 12, 2023 •

edited

Loading

	senderNonce, randKeySeen := r.senderNonces[randStr]
	senderNonces, randKeySeen := r.senderNonces[randStr]

Allow for out of order nonce #2710

Allow for out of order nonce #2710

Conversation

cyberj0g commented Jan 5, 2023

thomshutt Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

cyberj0g Jan 6, 2023

Choose a reason for hiding this comment

thomshutt left a comment

Choose a reason for hiding this comment

yondonfu Jan 6, 2023

Choose a reason for hiding this comment

yondonfu Jan 6, 2023

Choose a reason for hiding this comment

yondonfu Jan 6, 2023

Choose a reason for hiding this comment

Quintendos commented Jan 11, 2023

cyberj0g commented Jan 12, 2023

codecov bot commented Jan 12, 2023 • edited Loading

Codecov Report

yondonfu left a comment • edited Loading

Choose a reason for hiding this comment

yondonfu Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

yondonfu Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

thomshutt Jan 5, 2023 •

edited

Loading

codecov bot commented Jan 12, 2023 •

edited

Loading

yondonfu left a comment •

edited

Loading

yondonfu Jan 12, 2023 •

edited

Loading

yondonfu Jan 12, 2023 •

edited

Loading