Generalize expiry based de-duplication, dsmr #1810

tsachiherman · 2024-11-22T15:35:09Z

What ?

Integrate the generic de-deduplication logic into the dsmr

x/dsmr/block.go

x/dsmr/node.go

x/dsmr/node_test.go

tsachiherman · 2024-11-25T18:52:13Z

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: ***@***.***>

aaronbuchwald · 2024-11-29T17:53:18Z

internal/validitywindow/validitywindow.go

+	// 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
+	}


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.

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/block.go

x/dsmr/node.go

aaronbuchwald · 2024-11-29T18:04:03Z

x/dsmr/node.go

+	if block.Tmstmp <= parentBlock.Tmstmp && parentBlock.Hght > 0 {
+		return ErrTimestampNotMonotonicallyIncreasing
+	}
+


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.

aaronbuchwald · 2024-11-29T18:05:35Z

x/dsmr/node_test.go

+	if blk, has := ti.blocks[id]; has {
+		return blk, nil
+	}
+	return nil, nil


Fetching a non-existing block should return an error, can we return database.ErrNotFound if the block is not available?

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.

joshua-kim · 2024-12-03T18:37:42Z

internal/validitywindow/validitywindow.go

@@ -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()))


Can we use set.Set here?

joshua-kim · 2024-12-03T18:41:44Z

x/dsmr/certificate.go

+func (c ChunkCertificate) GetExpiry() int64 { return c.Expiry }

 func (c *ChunkCertificate) GetSlot() int64 { return c.Expiry }


I think these are duplicated - GetSlot is meant to be Expiry

joshua-kim · 2024-12-03T18:43:57Z

x/dsmr/node.go

+	log logging.Logger,
+	tracer trace.Tracer,
+	chainIndex ChainIndex,
+	validityWindowDuration int64,


Should we use time.Duration here?

joshua-kim · 2024-12-03T18:44:57Z

x/dsmr/node.go

+	oldestAllowed := timestamp - n.validityWindowDuration
+	if oldestAllowed < 0 {
+		oldestAllowed = 0
+	}


nit: min(0, oldestAllowed)

joshua-kim · 2024-12-03T18:46:12Z

x/dsmr/node_test.go

+	blocks map[ids.ID]validitywindow.ExecutionBlock[*ChunkCertificate]
+}
+
+func (ti *testingChainIndex) GetExecutionBlock(_ context.Context, id ids.ID) (validitywindow.ExecutionBlock[*ChunkCertificate], error) {


Nits:

receiver name should just be t

id -> blkID or blockID

joshua-kim · 2024-12-03T19:21:00Z

x/dsmr/block.go

 func (b Block) GetID() ids.ID {
 	return b.blkID
 }
+
+func (b Block) Parent() ids.ID {


I don't care which we pick, but we should be consistent on the naming of either Foo() or GetFoo().

I'm going to dodge this one by saying that this method is no longer needed.

joshua-kim · 2024-12-03T19:21:14Z

x/dsmr/block.go

+func (b Block) Txs() []*ChunkCertificate {
+	return b.ChunkCerts
+}


This seems weird because these are not returning txs

yes, I agree. let's discuss this as a group ?

Can we use the term containers?

joshua-kim · 2024-12-03T19:21:27Z

x/dsmr/node.go

+	log logging.Logger,
+	tracer trace.Tracer,


nit: make these (logger + tracer) the first args in this fn

joshua-kim · 2024-12-03T19:22:38Z

x/dsmr/node.go

+type (
+	ChainIndex         = validitywindow.ChainIndex[*ChunkCertificate]
+	timeValidityWindow = *validitywindow.TimeValidityWindow[*ChunkCertificate]
+)


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?

@aaronbuchwald asked to remove this section completely.

joshua-kim · 2024-12-03T19:26:11Z

x/dsmr/block.go

 	"github.com/ava-labs/hypersdk/utils"
 )

 const InitialChunkSize = 250 * 1024

 type Tx interface {
-	GetID() ids.ID
-	GetExpiry() int64
+	emap.Item


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.

once merged, we won't need wrapping interface anymore. . unless I'm missing something ?

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

aaronbuchwald · 2024-12-12T22:16:14Z

x/dsmr/block.go

+	return e.innerBlock.ParentID
+}
+
+func (e ExecutionBlock) Txs() []*emapChunkCertificate {


Can we rename this to a neutral name instead of txs like containers?

aaronbuchwald · 2024-12-17T18:15:22Z

internal/validitywindow/validitywindow.go

+	blkTxsIDs := set.NewSet[ids.ID](len(blk.Containers()))
+	for _, tx := range blk.Containers() {
+		id := tx.GetID()
+		if blkTxsIDs.Contains(id) {
+			return fmt.Errorf("%w: duplicate in block", ErrDuplicateContainer)
+		}
+		blkTxsIDs.Add(id)
+	}


Can we rename the local variables here?

aaronbuchwald · 2024-12-17T18:15:36Z

internal/validitywindow/validitywindow.go

@@ -110,7 +121,7 @@ func (v *TimeValidityWindow[Container]) isRepeat(
 			if marker.Contains(i) {
 				continue
 			}
-			if ancestorBlk.ContainsTx(tx.GetID()) {
+			if ancestorBlk.Contains(tx.GetID()) {


same comment on tx.GetID() here

aaronbuchwald · 2024-12-17T18:16:41Z

x/dsmr/block.go

+func (b Block) init() {
+	for _, c := range b.ChunkCerts {
+		b.certSet.Add(c.ChunkID)
+	}
+}


Can we move the cert set to the execution block type?

aaronbuchwald · 2024-12-17T18:29:53Z

x/dsmr/node_test.go

+			_, err := node.BuildBlock(context.Background(), parentBlk, testCase.timestamp)
+			r.ErrorIs(err, testCase.buildWantErr)
+
+			r.ErrorIs(node.Verify(context.Background(), parentBlk, newBlk), testCase.verifyWantErr)
+		})
+	}


This test case looks like it should pass because calling Accept should mark the block as accepted and make sure it's part of the index.

aaronbuchwald · 2024-12-17T18:37:01Z

x/dsmr/node_test.go

+	}
+	r.NoError(node.Accept(context.Background(), blk))
+
+	r.NoError(node.storage.AddLocalChunkWithCert(chunk, &chunkCert))


Agree with @joshua-kim here that it does not make sense to test this way since a user of this package should never call AddLocalChunkWithCert directly.

The way we'd hit this case is by attempting to build a block with a chunk that is in our pool, but has not been included in an accepted block yet (and therefore has not been removed from that pool).

aaronbuchwald · 2024-12-17T19:01:49Z

x/dsmr/node_test.go

+			nodes, indexer := newNodes(t, 1)
+			node = nodes[0]


I'm not sure the validity window should be separate from the nodes, since each node maintains its own index

it's not the validity window itself, but rather the validity window duration. The index remains the same.

The indexer is usually part of the node itself so sharing it across each node in tests seems like an anti-pattern

I could create an indexer per node, if you think it would be better.. but it would be a bit of over engineering solution given that it's being used to retrieve previously accepted blocks. Please let me know if you think it's the right move and I'll make it so.

aaronbuchwald · 2024-12-17T19:02:26Z

x/dsmr/node_test.go

+					context.Background(),
+					[]tx{
+						{
+							ID:     ids.Empty, // ids.GenerateTestID(),


Is this meant to be using ids.GenerateTestID() ?

no; but good catch. It needed to be a way to way to reproduce consistent duplicate chunks. I'd updated the code to use ids.Empty.Prefix(uint64(chunkExpiry))

aaronbuchwald · 2024-12-17T19:03:00Z

x/dsmr/node_test.go

+				parentBlk = blk
+			}
+
+			// create the block so that we can test it against the execute directly.


Suggested change

// create the block so that we can test it against the execute directly.

// create the block so that we can test it against Execute directly.

aaronbuchwald · 2024-12-17T19:28:45Z

x/dsmr/node_test.go

+					_, chunkCert, err := node.BuildChunk(
+						context.Background(),
+						[]tx{
+							{
+								ID:     ids.Empty,
+								Expiry: int64(chunkExpiry),
+							},
+						},
+						int64(chunkExpiry),
+						codec.Address{},
+					)


Should we generate actual IDs instead of ids.Empty since these will technically be invalid?

aaronbuchwald · 2024-12-17T19:30:29Z

x/dsmr/node_test.go

+			_, err := node.BuildBlock(context.Background(), parentBlk, testCase.timestamp)
+			r.ErrorIs(err, testCase.buildWantErr)
+
+			r.ErrorIs(node.Verify(context.Background(), parentBlk, newBlk), testCase.verifyWantErr)
+		})


If BuildBlock succeeds, it seems that we are expecting it to build the same block that we have produced locally. Should we be checking that they're the same here?

if the BuildBlock won't return any of the expected errors, than it would return a valid block. However, I'm not sure how easy it would be to compare it to the testing block we've created. I'll add ParentID, Height and Timestamp.

joshua-kim · 2024-12-19T18:10:41Z

x/dsmr/node_test.go

@@ -169,6 +197,38 @@ func TestNode_GetChunk_AvailableChunk(t *testing.T) {
 	<-done
 }

+func TestIndexerMissingBlock(t *testing.T) {


Isn't this a test over the duplicate chunk case?

no; after

r.NoError(node.Verify(context.Background(), node.LastAccepted, blk)) r.NoError(node.Accept(context.Background(), blk))

I intentionally did not add the newly accepted block to the indexer.
As a result, when

r.ErrorIs(node.Verify(context.Background(), node.LastAccepted, blkNext), database.ErrNotFound)

is called, it wouldn't have that block in the indexer, and would generate the database.ErrNotFound error.

as far as I can tell, this is the only test that we have that cover this case.
( obviously, it shouldn't happen on any real VM that index all it's accepted blocks )

joshua-kim · 2024-12-19T18:13:04Z

x/dsmr/node_test.go

+	blkNext, err := node.BuildBlock(context.Background(), node.LastAccepted, 4)
+	r.NoError(err)
+
+	r.ErrorIs(node.Verify(context.Background(), node.LastAccepted, blkNext), database.ErrNotFound)


Why are we getting database.ErrNotFound here? It seems to me like this would fail verification because this block references a chunk that contains duplicated txs

it wouldn't be the same transactions since ids.GenerateTestID() is random.

joshua-kim · 2024-12-19T20:43:01Z

x/dsmr/block.go

@@ -105,6 +106,47 @@ func ParseChunk[T Tx](chunkBytes []byte) (Chunk[T], error) {
 	return c, c.init()
 }

+// ExecutionBlock bridge the gap between the dsmr's block implementation and the validity window's execution block interface.
+type ExecutionBlock struct {


I think we should unexport this type since it's not needed downstream of dsmr, since we're only using this type within the validity window.

Can we call this something like validityWindowBlock? We're going to be introducing another ExecutedBlock type which will be too similar to this type name.

Sure, wasn't aware of that, and would gladly rename the data structure.

joshua-kim · 2024-12-19T20:45:45Z

x/dsmr/node_test.go


 	chainID = ids.Empty
 )

+type testValidityWindowChainIndex struct {


nit: move all testing types to bottom of file

joshua-kim · 2024-12-19T22:06:01Z

x/dsmr/node_test.go

+		Height:    1,
+		Timestamp: 1,
+		blkID:     ids.GenerateTestID(),
+	}


This block isn't valid, I think we need to call BuildChunk -> BuildBlock -> Verify before calling Accept. As an unrelated note I don't think we need this block at all for this test.

joshua-kim · 2024-12-20T16:14:08Z

x/dsmr/node_test.go

 	}

-	return result
+	return result, indexer


Do we want this indexer to be shared across all node instances?

I would prefer not since the "indexer" is unique per node in practice

joshua-kim · 2024-12-20T16:16:49Z

x/dsmr/node_test.go

+	}
+	r.NoError(node.Accept(context.Background(), blk))
+
+	_, err = node.BuildBlock(context.Background(), blk, 3)


nit: Use node.LastAccepted instead of referencing blk for the parent to build off of

joshua-kim · 2024-12-20T16:19:22Z

x/dsmr/node_test.go

+	// make sure that it's not the case with any other chunk.
+	_, _, err = node.BuildChunk(
+		context.Background(),
+		[]tx{
+			{
+				ID:     ids.GenerateTestID(),
+				Expiry: 4,
+			},
+		},
+		4,
+		codec.Address{},
+	)
+	r.NoError(err)
+	r.NoError(node.Accept(context.Background(), blk))


I feel like instead of having a single test case that checks:

Build a duplicate chunk, have it fail, and build a new chunk and see if it passes

It would just make more sense to have this split into separate test cases:

Build a Chunk, it should pass

Build a duplicate chunk, it should fail

Build a chunk, build a duplicate chunk, should pass with a single chunk in the block

joshua-kim · 2024-12-20T16:23:44Z

x/dsmr/node_test.go

+				blk := Block{
+					ParentID:  parentBlk.GetID(),
+					Height:    uint64(int(node.LastAccepted.Height) + 1),
+					Timestamp: int64(int(node.LastAccepted.Timestamp) + 1),
+					blkID:     ids.GenerateTestID(),
+				}
+
+				for _, chunkExpiry := range chunkList {
+					_, chunkCert, err := node.BuildChunk(
+						context.Background(),
+						[]tx{
+							{
+								ID:     ids.Empty.Prefix(uint64(chunkExpiry)),
+								Expiry: int64(chunkExpiry),
+							},
+						},
+						int64(chunkExpiry),
+						codec.Address{},
+					)
+					r.NoError(err)
+					blk.ChunkCerts = append(blk.ChunkCerts, &chunkCert)
+				}


Can we use BuildBlock after calling BuildChunk instead of initializing the block through the struct?

joshua-kim · 2024-12-20T16:26:37Z

x/dsmr/node_test.go

+				r.NoError(node.Verify(context.Background(), parentBlk, blk))
+
+				r.NoError(node.Accept(context.Background(), blk))
+				indexer.set(blk.GetID(), NewExecutionBlock(blk))


Should dsmr be updating the chain index? Or are we expecting that someone else is responsible for updating it (current behavior)?

The snow refactor separates the chain index from Accept, so it's reasonable imo to do the same thing here imo

aaronbuchwald · 2024-12-20T22:19:21Z

x/dsmr/node_test.go

+			nodes, indexer := newNodes(t, 1)
+			node = nodes[0]


The indexer is usually part of the node itself so sharing it across each node in tests seems like an anti-pattern

aaronbuchwald · 2024-12-20T22:20:09Z

x/dsmr/node_test.go

+			var node *Node[tx]
+			nodes, indexer := newNodes(t, 1)
+			node = nodes[0]
+			node.validityWindowDuration = validationWindow


Should this be an argument to create new nodes instead of setting it after constructing the nodes?

aaronbuchwald · 2024-12-20T22:21:32Z

x/dsmr/node_test.go

+				r.NoError(node.Verify(context.Background(), parentBlk, blk))
+
+				r.NoError(node.Accept(context.Background(), blk))
+				indexer.set(blk.GetID(), NewExecutionBlock(blk))


The snow refactor separates the chain index from Accept, so it's reasonable imo to do the same thing here imo

aaronbuchwald · 2024-12-20T22:21:51Z

x/dsmr/node_test.go

 	}

-	return result
+	return result, indexer


I would prefer not since the "indexer" is unique per node in practice

…s/hypersdk into tsachi/refactor_validity_window3

eliminate emapChunk

e5ecf69

tsachiherman self-assigned this Nov 22, 2024

tsachiherman added 2 commits November 22, 2024 11:52

update

ba644c3

update

2939543

aaronbuchwald reviewed Nov 25, 2024

View reviewed changes

x/dsmr/block.go Outdated Show resolved Hide resolved

x/dsmr/node.go Outdated Show resolved Hide resolved

x/dsmr/node.go Outdated Show resolved Hide resolved

x/dsmr/node.go Outdated Show resolved Hide resolved

x/dsmr/node.go Outdated Show resolved Hide resolved

tsachiherman added 6 commits November 25, 2024 10:09

Merge branch 'main' into tsachi/refactor_validity_window3

08f0d59

update per CR

659f1fb

lint

921b908

apply requested changes.

508cc92

fix code + test.

7a3fed6

rollback unneeded changes.

b312a3c

tsachiherman marked this pull request as ready for review November 25, 2024 18:20

tsachiherman requested a review from joshua-kim as a code owner November 25, 2024 18:20

rename testing chain indexer.

1238460

aaronbuchwald reviewed Nov 25, 2024

View reviewed changes

x/dsmr/node.go Outdated Show resolved Hide resolved

x/dsmr/node_test.go Outdated Show resolved Hide resolved

x/dsmr/node_test.go Outdated Show resolved Hide resolved

x/dsmr/node_test.go Outdated Show resolved Hide resolved

tsachiherman added 8 commits November 25, 2024 15:04

update

8c1ab98

add unit test.

f8e42a8

update unit test.

b000edf

Merge branch 'main' into tsachi/refactor_validity_window3

1d2536e

lint

6911f26

update

f2816e7

lint

e78f847

update

270245a

aaronbuchwald reviewed Nov 29, 2024

View reviewed changes

joshua-kim mentioned this pull request Dec 2, 2024

Generate Warp Signatures #1818

Merged

joshua-kim reviewed Dec 3, 2024

View reviewed changes

tsachiherman added 3 commits December 5, 2024 11:25

Merge branch 'main' into tsachi/refactor_validity_window3

144ddd4

fix few CR feedback.

2fa701d

add certSet

2220b5e

tsachiherman added 11 commits December 10, 2024 11:04

update

fc87f31

update

cf82891

updatge

864acc1

fix typo

d96160e

undo unwanted changes

9ea09b4

update

30bcdce

update

8d52c05

update unit test

32207b9

update test

5d62af7

update

7ca779b

update

383a7ef

tsachiherman requested review from joshua-kim and aaronbuchwald December 12, 2024 22:10

aaronbuchwald reviewed Dec 12, 2024

View reviewed changes

update per review feedback,.

7902a6a

aaronbuchwald reviewed Dec 17, 2024

View reviewed changes

aaronbuchwald mentioned this pull request Dec 18, 2024

Bonds for Fortified DSMR #1835

Merged

tsachiherman added 3 commits December 19, 2024 13:14

address feedback.

1f143ea

update per cr

6d4f94c

Merge branch 'main' into tsachi/refactor_validity_window3

dd419bb

tsachiherman requested a review from aaronbuchwald December 20, 2024 14:14

joshua-kim reviewed Dec 20, 2024

View reviewed changes

aaronbuchwald reviewed Dec 20, 2024

View reviewed changes

tsachiherman added 7 commits December 23, 2024 11:21

update

c87b033

Merge branch 'tsachi/refactor_validity_window3' of github.com:ava-lab…

293c6b9

…s/hypersdk into tsachi/refactor_validity_window3

update per CR feedback.

b501cec

update per CR

68603db

update per feedback.

7e72626

update

7edef6e

Merge branch 'main' into tsachi/refactor_validity_window3

185fbf0

		func (c ChunkCertificate) GetExpiry() int64 { return c.Expiry }

		func (c *ChunkCertificate) GetSlot() int64 { return c.Expiry }

	// create the block so that we can test it against the execute directly.
	// create the block so that we can test it against Execute directly.

Generalize expiry based de-duplication, dsmr #1810

Are you sure you want to change the base?

Generalize expiry based de-duplication, dsmr #1810

Conversation

tsachiherman commented Nov 22, 2024 • edited Loading

What ?

tsachiherman commented Nov 25, 2024 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronbuchwald Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsachiherman Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsachiherman commented Nov 22, 2024 •

edited

Loading

aaronbuchwald Dec 12, 2024 •

edited

Loading

tsachiherman Dec 23, 2024 •

edited

Loading