fix: Add entropy to counter CRDT type updates #2186
Conversation
fredcarle
changed the title
| func (reg PNCounter[T]) Increment(ctx context.Context, value T) (*PNCounterDelta[T], error) { | ||
| // To ensure that the dag block is unique, we add a random number to the delta. | ||
| // This is done only on update (if the doc doesn't already exist) to ensure that the | ||
| // initial dag block of a document can be reproducible. |
There was a problem hiding this comment.
praise: Good catch :)
question: Do we have any tests that target this and ensure that the docID is consistent (e.g. by hard coding it in the test)? If not, please consider this a todo :)
There was a problem hiding this comment.
Should have read first, there are tests that cover this, but they are complicated by cids and updates. Can you please add a simple one that makes it clear why such a test is needed?
There was a problem hiding this comment.
If the docID wasn't consistent, the updates wouldn't sync over P2P.
There was a problem hiding this comment.
True :) Thanks for pointing out
There was a problem hiding this comment.
I'm adding a docID assertion on the mutation create test just to make it more obvious.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #2186 +/- ##
===========================================
+ Coverage 74.16% 74.28% +0.13%
===========================================
Files 256 256
Lines 25479 25497 +18
===========================================
+ Hits 18894 18940 +46
+ Misses 5292 5271 -21
+ Partials 1293 1286 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
core/crdt/pncounter.go
Outdated
| @@ -45,6 +48,9 @@ type PNCounterDelta[T Incrementable] struct { | |||
| DocID []byte | |||
| FieldName string | |||
| Priority uint64 | |||
| // Entropy is an added randomly generated number that ensures | |||
| // that each increment operation is unique. | |||
| Entropy int64 | |||
There was a problem hiding this comment.
todo: Lets rename this to Nonce as its more cryptographically accurate, and slightly more common
There was a problem hiding this comment.
Lol... Whoever introduced that to crypto was probably not British... It is not very polite. If it is only slightly more common I'd suggest sticking with Entropy.
There was a problem hiding this comment.
FWIW nonce is fairly common in the cryptography world, so I have a slight pref for it.
| return nil, err | ||
| } | ||
| var entropy int64 | ||
| if exists { |
There was a problem hiding this comment.
question: When creating a new PNCounter field, this is going to produce an entropy value of 0. Is this intended? Or do we want to omit the entropy field entirely when creating? (IE: not include in the cbor marshal)
There was a problem hiding this comment.
This is indeed intended. Instead of having a different struct for create vs update, using 0 gives us the same result with less complexity.
There was a problem hiding this comment.
A clarification. The entropy will be 0 only when creating the document.
fredcarle
force-pushed
the
fredcarle/fix/I2179-counter-entropy
branch
2 times, most recently
from
f5b6019 to
1485530
Compare
fredcarle
force-pushed
the
fredcarle/fix/I2179-counter-entropy
branch
from
472dc2c to
c7399bc
Compare
## Relevant issue(s) Resolves sourcenetwork#2179 ## Description Counters need to be incrementable independently from different nodes. Initially, if two nodes incremented by the same amount from the same synced state, then the update CID would be the same and thus taken as a the same operation when syncing. So starting from 10 if both node A and B increment by 5 simultaneously, because they would generate the same CID for the update, the end result would be 15. We want it to be 20. This PR adds entropy to the PN Counter delta so that a situation like described above would result in 20. Note that the entropy is only added on update so that a document can be instantiated on multiple nodes with exactly the same CID.
## Relevant issue(s) Resolves sourcenetwork#2179 ## Description Counters need to be incrementable independently from different nodes. Initially, if two nodes incremented by the same amount from the same synced state, then the update CID would be the same and thus taken as a the same operation when syncing. So starting from 10 if both node A and B increment by 5 simultaneously, because they would generate the same CID for the update, the end result would be 15. We want it to be 20. This PR adds entropy to the PN Counter delta so that a situation like described above would result in 20. Note that the entropy is only added on update so that a document can be instantiated on multiple nodes with exactly the same CID.
Relevant issue(s)
Resolves #2179
Description
Counters need to be incrementable independently from different nodes. Initially, if two nodes incremented by the same amount from the same synced state, then the update CID would be the same and thus taken as a the same operation when syncing. So starting from 10 if both node A and B increment by 5 simultaneously, because they would generate the same CID for the update, the end result would be 15. We want it to be 20.
This PR adds entropy to the PN Counter delta so that a situation like described above would result in 20.
Note that the entropy is only added on update so that a document can be instantiated on multiple nodes with exactly the same CID.
Tasks
How has this been tested?
make test
Specify the platform(s) on which this was tested: