-
Notifications
You must be signed in to change notification settings - Fork 118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Generalize expiry based de-duplication, dsmr #1810
Conversation
All sounds like great ideas, thanks!
…On Mon, Nov 25, 2024, 1:27 PM aaronbuchwald ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In x/dsmr/node.go
<#1810 (comment)>:
> @@ -172,19 +193,28 @@ func (n *Node[T]) BuildChunk(
return chunk, n.storage.AddLocalChunkWithCert(chunk, &chunkCert)
}
-func (n *Node[T]) BuildBlock(parent Block, timestamp int64) (Block, error) {
- if timestamp <= parent.Timestamp {
+// BuildBlock(ctx context.Context, parentView state.View, parent *ExecutionBlock) (*ExecutionBlock, *ExecutedBlock, merkledb.View, error)
can we remove this comment?
------------------------------
In x/dsmr/node_test.go
<#1810 (comment)>:
> +func newTestingLog() logging.Logger {
+ return logging.NewLogger("dsmr")
+}
we can replacing this with logging.NoLog{}
------------------------------
In x/dsmr/node_test.go
<#1810 (comment)>:
> +func newTestingTracer(t *testing.T) trace.Tracer {
+ tracer, err := trace.New(trace.Config{})
+ require.NoError(t, err)
+ return tracer
+}
we can replace this with trace.Noop
------------------------------
In x/dsmr/node_test.go
<#1810 (comment)>:
> +type testingChainIndex struct{}
+
+func (*testingChainIndex) GetExecutionBlock(context.Context, ids.ID) (validitywindow.ExecutionBlock[*ChunkCertificate], error) {
+ return nil, nil
+}
+
+func newChainIndexer() ChainIndex {
+ return &testingChainIndex{}
+}
We should add unit tests for the newly added functionality here. What do
you think of the following cases?
- skip duplicate chunks in build block
- return an error if all available chunks are duplicates in build block
- duplicate chunk within a block
- duplicate chunk that between the block we are executing and the last
accepted block
- duplicate chunk inside of the accepted validity window
- propagate an error if we fail to fetch a block during de-duplication
—
Reply to this email directly, view it on GitHub
<#1810 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AF2OOH6AKK7BDRFH2ESZGB32CNT2VAVCNFSM6AAAAABSJTMZI2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINJZGMZDKMZVGM>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
// make sure we have no repeats within the block itself. | ||
blkTxsIDs := make(map[ids.ID]bool, len(blk.Txs())) | ||
for _, tx := range blk.Txs() { | ||
id := tx.GetID() | ||
if _, has := blkTxsIDs[id]; has { | ||
return fmt.Errorf("%w: duplicate in block", ErrDuplicateContainer) | ||
} | ||
blkTxsIDs[id] = true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do this check twice now in chain since we previously handled this within NewExecutionBlock
We should only apply the check once, which do you think is the better place for it? The other addition within NewExecutionBlock
is pre-populating the signature job. It would probably be better to remove that from the execution block and handle it in AsyncVerify
. One good reason to do this is that handling this within ParseBlock
can be a DoS vector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why testing this twice is an issue on it's own, and I think that I have a better solution here;
Testing it in NewExecutionBlock is not ideal from my perspective, as it should be construction step ( i.e. no errors ).
How do you feel about the following:
in ExecutionBlock, we would add the following function:
func (b *ExecutionBlock) ValidateDuplicateTransactions() error {
if len(b.Txs) != b.txsSet.Len() {
ErrDuplicateTx
}
return nil
than in validitywindow.go VerifyExpiryReplayProtection
, we can just call this method:
...
if err := blk.ValidateDuplicateTransactions(); err != nil {
return err
}
...
and remove the test from NewExecutionBlock
x/dsmr/node.go
Outdated
if block.Tmstmp <= parentBlock.Tmstmp && parentBlock.Hght > 0 { | ||
return ErrTimestampNotMonotonicallyIncreasing | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this check from this PR?
We should enforce timestamp >= parent timestamp (should allow them to be equal in case a block builder pushes the timestamp ahead of wall clock time for some nodes.
We should not allow the case that a malicious node builds a block with a timestamp that we consider valid less than 1s ahead of our wall clock time, but still ahead of our wall clock time, such that when we build a block on top of it, we fail because the current timestamp is ahead of our local timestamp.
We should update the check applied in BuildBlock
imo.
We should also never execute the genesis block, so the check for parentBlock.Hght > 0
should be removed.
x/dsmr/node_test.go
Outdated
if blk, has := ti.blocks[id]; has { | ||
return blk, nil | ||
} | ||
return nil, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fetching a non-existing block should return an error, can we return database.ErrNotFound
if the block is not available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the ux of us using the sentinel database.ErrNotFound
experience to be very awkward. This is due to the existing interface definition and not the changes in this PR though.... if we had something like GetExecutionBlock() (ExecutionBlock[T], bool, error)
we wouldn't have to introspect the error and could just return an error if error
is non-nil and check the bool
if it just wasn't there.
@@ -69,6 +70,15 @@ func (v *TimeValidityWindow[Container]) VerifyExpiryReplayProtection( | |||
if dup.Len() > 0 { | |||
return fmt.Errorf("%w: duplicate in ancestry", ErrDuplicateContainer) | |||
} | |||
// make sure we have no repeats within the block itself. | |||
blkTxsIDs := make(map[ids.ID]bool, len(blk.Txs())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use set.Set
here?
x/dsmr/certificate.go
Outdated
func (c ChunkCertificate) GetExpiry() int64 { return c.Expiry } | ||
|
||
func (c *ChunkCertificate) GetSlot() int64 { return c.Expiry } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these are duplicated - GetSlot
is meant to be Expiry
x/dsmr/node.go
Outdated
log logging.Logger, | ||
tracer trace.Tracer, | ||
chainIndex ChainIndex, | ||
validityWindowDuration int64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use time.Duration
here?
x/dsmr/node.go
Outdated
oldestAllowed := timestamp - n.validityWindowDuration | ||
if oldestAllowed < 0 { | ||
oldestAllowed = 0 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: min(0, oldestAllowed)
x/dsmr/node_test.go
Outdated
blocks map[ids.ID]validitywindow.ExecutionBlock[*ChunkCertificate] | ||
} | ||
|
||
func (ti *testingChainIndex) GetExecutionBlock(_ context.Context, id ids.ID) (validitywindow.ExecutionBlock[*ChunkCertificate], error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nits:
- receiver name should just be
t
id
->blkID
orblockID
x/dsmr/block.go
Outdated
func (b Block) GetID() ids.ID { | ||
return b.blkID | ||
} | ||
|
||
func (b Block) Parent() ids.ID { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't care which we pick, but we should be consistent on the naming of either Foo()
or GetFoo()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to dodge this one by saying that this method is no longer needed.
x/dsmr/block.go
Outdated
func (b Block) Txs() []*ChunkCertificate { | ||
return b.ChunkCerts | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems weird because these are not returning txs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I agree. let's discuss this as a group ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the term containers?
x/dsmr/node.go
Outdated
log logging.Logger, | ||
tracer trace.Tracer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: make these (logger + tracer) the first args in this fn
x/dsmr/node.go
Outdated
type ( | ||
ChainIndex = validitywindow.ChainIndex[*ChunkCertificate] | ||
timeValidityWindow = *validitywindow.TimeValidityWindow[*ChunkCertificate] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we're doing this to avoid the caller depending on an internal package. I'm wondering if it even makes sense for validitywindow
to be in internal at all... should this just be merged into dsmr
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaronbuchwald asked to remove this section completely.
x/dsmr/block.go
Outdated
"github.com/ava-labs/hypersdk/utils" | ||
) | ||
|
||
const InitialChunkSize = 250 * 1024 | ||
|
||
type Tx interface { | ||
GetID() ids.ID | ||
GetExpiry() int64 | ||
emap.Item |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exposes the internal/emap
package into the caller. I think the previous wrapping pattern where we wrapped this interface w/ a type that implemented the emap interface actually looked cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once merged, we won't need wrapping interface anymore. . unless I'm missing something ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @joshua-kim 's point is to have the internal type implemented as makes sense in this package and then wrap it with another type that converts between that structure and the required interface when we need to use it in the validity window or expiry map where we need a specific interface. Correct me if I'm wrong @joshua-kim
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline, can we re-write the tests with the following assumptions:
- validity window is a separate component from DSMR in practice and will be responsible for keeping itself up to date
- therefore, it should be tested independently as unit tests for the validity window and within integration tests when used in conjunction with DSMR/chain
- we should add test cases for DSMR package that treat the validity window as a blackbox ie. return a mocked error from VerifyExpiryReplayProtection and IsRepeat and a mocked value of duplicates from IsRepeat that indicates one of the chunks is a duplicate and should be eliminated
- DSMR tests should not be responsible for setting the execution block index (treated as a separate component)
|
||
func int64ToID(n int64) ids.ID { | ||
var id ids.ID | ||
binary.LittleEndian.PutUint64(id[0:8], uint64(n)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use BigEndian
as we use it throughout the rest of the repo?
return false | ||
} | ||
|
||
func NewExecutionBlock(parent int64, timestamp int64, height uint64, contrainers ...int64) ExecutionBlock { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we fix the typo contrainers
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This constructor is fairly difficult to read in the tests as it is a sequence of similar numbers including a variadic final parameter. Could we switch the last argument to being a slice, so that it's clearer to read which numbers are the containers?
return c.Expiry | ||
} | ||
|
||
func NewContainer(expiry int64) Container { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this package to the validity window test file if this test package is not required externally?
// method for returning an id of the item | ||
func (c Container) GetID() ids.ID { | ||
return c.ID | ||
} | ||
|
||
// method for returning this items timestamp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove comments that do not add any new information?
} | ||
} | ||
|
||
// testing structures. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we remove comments that do not add any new information?
x/dsmr/block.go
Outdated
func (e validityWindowBlock) Timestamp() int64 { | ||
return e.Block.Timestamp | ||
} | ||
|
||
func (e validityWindowBlock) Height() uint64 { | ||
return e.Block.Height | ||
} | ||
|
||
func (e validityWindowBlock) Contains(id ids.ID) bool { | ||
return e.certs.Contains(id) | ||
} | ||
|
||
func (e validityWindowBlock) Parent() ids.ID { | ||
return e.Block.ParentID | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add those functions directly to the block type? It will need to implement them as well as part of the snow refactor where it must fulfill the snow.Block
type
x/dsmr/node_test.go
Outdated
4, | ||
codec.Address{}, | ||
)) | ||
_, err = node.Accept(context.Background(), blk) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should never call Accept
on the same block twice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. This Accept was not needed, so I've just dropped it.
x/dsmr/node_test.go
Outdated
r.NoError(node.BuildChunk( | ||
context.Background(), | ||
[]dsmrtest.Tx{ | ||
{ | ||
ID: ids.GenerateTestID(), | ||
Expiry: 4, | ||
}, | ||
}, | ||
4, | ||
codec.Address{}, | ||
)) | ||
|
||
blk, err := node.BuildBlock(context.Background(), node.LastAccepted, 2) | ||
r.NoError(err) | ||
r.NoError(node.Verify(context.Background(), node.LastAccepted, blk)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is testing what it's intended to. This should test that the validity window correctly eliminates chunks that are in the chunk pool from block building. However, we build, verify, and accept the block, which will remove the chunk from the chunk pool. Therefore, no chunks are actually "eliminated" by the validity window in this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added coverage for that in TestNode_BuildBlock_IncludesChunks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current flow here is build -> verify -> accept and then build another block and confirm that the chunks included in the already accepted block are correctly eliminated so BuildBlock
returns the expected error.
At that point, the node is in a state of having an empty chunk pool and an updated last accepted block. I guess this is just testing that BuildBlock
does not build an invalid block with the same chunk that was already included. Isn't that the same as the no available chunk certs in the test above?
…s/hypersdk into tsachi/refactor_validity_window3
Definitely doable, but would require passing in the underlying |
// make sure we have no repeats within the block itself. | ||
blkContainerIDs := set.NewSet[ids.ID](len(blk.Containers())) | ||
for _, container := range blk.Containers() { | ||
id := container.GetID() | ||
if blkContainerIDs.Contains(id) { | ||
return fmt.Errorf("%w: duplicate in block", ErrDuplicateContainer) | ||
} | ||
blkContainerIDs.Add(id) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change makes it inconsistent across tx deduplication and chunk deduplication how this is handled. Currently tx deduplication will make sure that there are no duplicate transactions included in a block when it parses or creates it, so that the validitywindow package does not need to check for duplicates within the requested block.
We should align on one way or the other. I think it's a smaller change to this PR to go with the current style for execution block of checking for duplicates when parsing/creating the block, but think either version is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're correct that it's being performed twice : one in validitywindow.VerifyExpiryReplayProtection
and one in chain.NewExecutionBlock
. Yet, the functionality is missing in dsmr.NewValidityWindowBlock
.
This functionality is logically needed for the correctness of the replay protection, although an early testing would work just as well.
my preference would be to keep it in validitywindow so that we can claim that the entire replay-protection is encapsulated in one place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, could we remove the check for this in ExecutionBlock
in that case, so that we don't duplicate the check?
Name string | ||
Accepted []executionBlock | ||
VerifyBlock executionBlock | ||
OldestAllowed int64 | ||
ExpectedError error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we unexport the field names here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add an Accepted
field to indicate the last block considered accepted as opposed to in processing as we do for TestValidityWindowIsRepeat
below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure exactly how to do that, since we really need the included chunks to be set correctly, as it's being tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we do the exact same as we do below and just test VerifyExpiryReplayProtection
instead of IsRepeat
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r := require.New(t)
chainIndex := &testChainIndex{}
tvw := NewTimeValidityWindow(&logging.NoLog{}, trace.Noop, chainIndex)
r.NotNil(tvw)
for i, blk := range test.blocks {
if i <= int(test.accepted) {
tvw.Accept(blk)
}
chainIndex.set(blk.GetID(), blk)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh - now I understood what you meant. You want that the last entry in the blocks would be the one used for the VerifyExpiryReplayProtection
x/dsmr/node_test.go
Outdated
r.NoError(node.BuildChunk( | ||
context.Background(), | ||
[]dsmrtest.Tx{ | ||
{ | ||
ID: ids.GenerateTestID(), | ||
Expiry: 4, | ||
}, | ||
}, | ||
4, | ||
codec.Address{}, | ||
)) | ||
|
||
blk, err := node.BuildBlock(context.Background(), node.LastAccepted, 2) | ||
r.NoError(err) | ||
r.NoError(node.Verify(context.Background(), node.LastAccepted, blk)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current flow here is build -> verify -> accept and then build another block and confirm that the chunks included in the already accepted block are correctly eliminated so BuildBlock
returns the expected error.
At that point, the node is in a state of having an empty chunk pool and an updated last accepted block. I guess this is just testing that BuildBlock
does not build an invalid block with the same chunk that was already included. Isn't that the same as the no available chunk certs in the test above?
No, it's not exactly the same. Please note of two things:
That allows us to test that the source for the error is the absence of chunks. ( note that all the chunks in this test have expiry of 4, far beyond the block time ). |
Co-authored-by: aaronbuchwald <[email protected]> Signed-off-by: Tsachi Herman <[email protected]>
Co-authored-by: aaronbuchwald <[email protected]> Signed-off-by: Tsachi Herman <[email protected]>
…s/hypersdk into tsachi/refactor_validity_window3
What ?
Integrate the generic de-deduplication logic into the dsmr