From b6cf776693d9c1f559adadf513db0fbe5fca8fde Mon Sep 17 00:00:00 2001 From: Andrea V <1577639+karimodm@users.noreply.github.com> Date: Wed, 4 Oct 2023 16:01:52 +0200 Subject: [PATCH] Address review comments --- .../engine/booker/inmemorybooker/booker.go | 4 +-- pkg/protocol/engine/ledger/ledger/ledger.go | 2 +- .../engine/mempool/tests/testframework.go | 2 +- pkg/protocol/engine/mempool/tests/tests.go | 20 ++++++------- .../engine/mempool/transaction_metadata.go | 6 ++-- .../engine/mempool/v1/inclusion_flags.go | 28 +++++++++---------- pkg/protocol/engine/mempool/v1/mempool.go | 6 ++-- .../engine/mempool/v1/state_metadata.go | 4 +-- .../engine/mempool/v1/transaction_metadata.go | 12 ++++---- pkg/testsuite/transactions.go | 2 +- 10 files changed, 42 insertions(+), 44 deletions(-) diff --git a/pkg/protocol/engine/booker/inmemorybooker/booker.go b/pkg/protocol/engine/booker/inmemorybooker/booker.go index e24fa8259..9f1b62066 100644 --- a/pkg/protocol/engine/booker/inmemorybooker/booker.go +++ b/pkg/protocol/engine/booker/inmemorybooker/booker.go @@ -115,7 +115,7 @@ func (b *Booker) Queue(block *blocks.Block) error { signedTransactionMetadata.OnSignaturesValid(func() { transactionMetadata := signedTransactionMetadata.TransactionMetadata() - if orphanedSlot, isOrphaned := transactionMetadata.IsOrphaned(); isOrphaned && orphanedSlot <= block.SlotCommitmentID().Slot() { + if orphanedSlot, isOrphaned := transactionMetadata.GetOrphanedSlot(); isOrphaned && orphanedSlot <= block.SlotCommitmentID().Slot() { block.SetInvalid() return @@ -158,7 +158,7 @@ func (b *Booker) book(block *blocks.Block) error { return ierrors.Errorf("failed to load transaction %s for block %s", conflictID.String(), block.ID()) } - if orphanedSlot, orphaned := txMetadata.IsOrphaned(); orphaned && orphanedSlot <= block.SlotCommitmentID().Slot() { + if orphanedSlot, orphaned := txMetadata.GetOrphanedSlot(); orphaned && orphanedSlot <= block.SlotCommitmentID().Slot() { // Merge-to-master orphaned conflicts. conflictsToInherit.Delete(conflictID) } diff --git a/pkg/protocol/engine/ledger/ledger/ledger.go b/pkg/protocol/engine/ledger/ledger/ledger.go index 2576ecb36..bddd760d4 100644 --- a/pkg/protocol/engine/ledger/ledger/ledger.go +++ b/pkg/protocol/engine/ledger/ledger/ledger.go @@ -194,7 +194,7 @@ func (l *Ledger) CommitSlot(slot iotago.SlotIndex) (stateRoot iotago.Identifier, // Mark each transaction as committed so the mempool can evict it stateDiff.ExecutedTransactions().ForEach(func(_ iotago.TransactionID, tx mempool.TransactionMetadata) bool { - tx.Commit(slot) + tx.Commit() return true }) diff --git a/pkg/protocol/engine/mempool/tests/testframework.go b/pkg/protocol/engine/mempool/tests/testframework.go index a453bf68f..71976e432 100644 --- a/pkg/protocol/engine/mempool/tests/testframework.go +++ b/pkg/protocol/engine/mempool/tests/testframework.go @@ -158,7 +158,7 @@ func (t *TestFramework) CommitSlot(slot iotago.SlotIndex) { }) stateDiff.ExecutedTransactions().ForEach(func(_ iotago.TransactionID, transaction mempool.TransactionMetadata) bool { - transaction.Commit(slot) + transaction.Commit() return true }) diff --git a/pkg/protocol/engine/mempool/tests/tests.go b/pkg/protocol/engine/mempool/tests/tests.go index 962965cda..22a074d34 100644 --- a/pkg/protocol/engine/mempool/tests/tests.go +++ b/pkg/protocol/engine/mempool/tests/tests.go @@ -235,9 +235,9 @@ func TestSetInclusionSlot(t *testing.T, tf *TestFramework) { tf.CommitSlot(3) tf.RequireTransactionsEvicted(transactionDeletionState) - require.False(t, lo.Return2(tx1Metadata.IsOrphaned())) - require.False(t, lo.Return2(tx2Metadata.IsOrphaned())) - require.False(t, lo.Return2(tx3Metadata.IsOrphaned())) + require.False(t, lo.Return2(tx1Metadata.GetOrphanedSlot())) + require.False(t, lo.Return2(tx2Metadata.GetOrphanedSlot())) + require.False(t, lo.Return2(tx3Metadata.GetOrphanedSlot())) tf.RequireAttachmentsEvicted(lo.MergeMaps(attachmentDeletionState, map[string]bool{"block1": true, "block2": true, "block3": false})) } @@ -274,7 +274,7 @@ func TestSetTransactionOrphanage(t *testing.T, tf *TestFramework) { // We only evict after MCA tf.RequireTransactionsEvicted(map[string]bool{"tx1": false, "tx2": false, "tx3": false}) - require.True(t, lo.Return2(tx1Metadata.IsOrphaned())) + require.True(t, lo.Return2(tx1Metadata.GetOrphanedSlot())) require.True(t, tx2Metadata.IsPending()) require.True(t, tx3Metadata.IsPending()) @@ -312,9 +312,9 @@ func TestSetTxOrphanageMultipleAttachments(t *testing.T, tf *TestFramework) { require.False(t, tx3Metadata.IsAccepted()) tf.Instance.Evict(1) - require.False(t, lo.Return2(tx1Metadata.IsOrphaned())) - require.False(t, lo.Return2(tx2Metadata.IsOrphaned())) - require.False(t, lo.Return2(tx3Metadata.IsOrphaned())) + require.False(t, lo.Return2(tx1Metadata.GetOrphanedSlot())) + require.False(t, lo.Return2(tx2Metadata.GetOrphanedSlot())) + require.False(t, lo.Return2(tx3Metadata.GetOrphanedSlot())) require.True(t, lo.Return2(tf.ConflictDAG.ConflictSets(tf.TransactionID("tx1")))) require.True(t, lo.Return2(tf.ConflictDAG.ConflictSets(tf.TransactionID("tx2")))) @@ -322,9 +322,9 @@ func TestSetTxOrphanageMultipleAttachments(t *testing.T, tf *TestFramework) { tf.Instance.Evict(2) - require.True(t, lo.Return2(tx1Metadata.IsOrphaned())) - require.True(t, lo.Return2(tx2Metadata.IsOrphaned())) - require.True(t, lo.Return2(tx3Metadata.IsOrphaned())) + require.True(t, lo.Return2(tx1Metadata.GetOrphanedSlot())) + require.True(t, lo.Return2(tx2Metadata.GetOrphanedSlot())) + require.True(t, lo.Return2(tx3Metadata.GetOrphanedSlot())) // All conflicts still exist, as they are kept around until MCA require.True(t, lo.Return2(tf.ConflictDAG.ConflictSets(tf.TransactionID("tx1")))) diff --git a/pkg/protocol/engine/mempool/transaction_metadata.go b/pkg/protocol/engine/mempool/transaction_metadata.go index 12169467b..6fb9f5d09 100644 --- a/pkg/protocol/engine/mempool/transaction_metadata.go +++ b/pkg/protocol/engine/mempool/transaction_metadata.go @@ -17,7 +17,7 @@ type TransactionMetadata interface { ConflictIDs() reactive.Set[iotago.TransactionID] - Commit(slot iotago.SlotIndex) + Commit() IsSolid() bool @@ -57,7 +57,7 @@ type inclusionFlags interface { OnAccepted(callback func()) - IsCommitted() (slot iotago.SlotIndex, isCommitted bool) + GetCommittedSlot() (slot iotago.SlotIndex, isCommitted bool) OnCommitted(callback func(slot iotago.SlotIndex)) @@ -65,7 +65,7 @@ type inclusionFlags interface { OnRejected(callback func()) - IsOrphaned() (slot iotago.SlotIndex, isOrphaned bool) + GetOrphanedSlot() (slot iotago.SlotIndex, isOrphaned bool) OnOrphaned(callback func(slot iotago.SlotIndex)) } diff --git a/pkg/protocol/engine/mempool/v1/inclusion_flags.go b/pkg/protocol/engine/mempool/v1/inclusion_flags.go index f2cc13e42..22352fa74 100644 --- a/pkg/protocol/engine/mempool/v1/inclusion_flags.go +++ b/pkg/protocol/engine/mempool/v1/inclusion_flags.go @@ -11,24 +11,24 @@ type inclusionFlags struct { // accepted gets triggered when the entity gets marked as accepted. accepted reactive.Variable[bool] - // committed gets set when the entity gets marked as committed. - committed reactive.Variable[iotago.SlotIndex] + // committedOnSlot gets set to the slot in which the entity gets marked as committed. + committedOnSlot reactive.Variable[iotago.SlotIndex] // rejected gets triggered when the entity gets marked as rejected. rejected *promise.Event - // orphaned gets set when the entity gets marked as orphaned. - orphaned reactive.Variable[iotago.SlotIndex] + // orphanedOnSlot gets set to the slot in which the entity gets marked as orphaned. + orphanedOnSlot reactive.Variable[iotago.SlotIndex] } // newInclusionFlags creates a new inclusionFlags instance. func newInclusionFlags() *inclusionFlags { return &inclusionFlags{ - accepted: reactive.NewVariable[bool](), - committed: reactive.NewVariable[iotago.SlotIndex](), - rejected: promise.NewEvent(), + accepted: reactive.NewVariable[bool](), + committedOnSlot: reactive.NewVariable[iotago.SlotIndex](), + rejected: promise.NewEvent(), // Make sure the oldest orphaned index doesn't get overridden by newer TX spending the orphaned conflict further. - orphaned: reactive.NewVariable[iotago.SlotIndex](func(currentValue, newValue iotago.SlotIndex) iotago.SlotIndex { + orphanedOnSlot: reactive.NewVariable[iotago.SlotIndex](func(currentValue, newValue iotago.SlotIndex) iotago.SlotIndex { if currentValue != 0 { return currentValue } @@ -76,25 +76,25 @@ func (s *inclusionFlags) OnRejected(callback func()) { } // IsCommitted returns true if the entity was committed. -func (s *inclusionFlags) IsCommitted() (slot iotago.SlotIndex, isCommitted bool) { - return s.committed.Get(), s.committed.Get() != 0 +func (s *inclusionFlags) GetCommittedSlot() (slot iotago.SlotIndex, isCommitted bool) { + return s.committedOnSlot.Get(), s.committedOnSlot.Get() != 0 } // OnCommitted registers a callback that gets triggered when the entity gets committed. func (s *inclusionFlags) OnCommitted(callback func(slot iotago.SlotIndex)) { - s.committed.OnUpdate(func(_, newValue iotago.SlotIndex) { + s.committedOnSlot.OnUpdate(func(_, newValue iotago.SlotIndex) { callback(newValue) }) } // IsOrphaned returns true if the entity was orphaned. -func (s *inclusionFlags) IsOrphaned() (slot iotago.SlotIndex, isOrphaned bool) { - return s.orphaned.Get(), s.orphaned.Get() != 0 +func (s *inclusionFlags) GetOrphanedSlot() (slot iotago.SlotIndex, isOrphaned bool) { + return s.orphanedOnSlot.Get(), s.orphanedOnSlot.Get() != 0 } // OnOrphaned registers a callback that gets triggered when the entity gets orphaned. func (s *inclusionFlags) OnOrphaned(callback func(slot iotago.SlotIndex)) { - s.orphaned.OnUpdate(func(_, newValue iotago.SlotIndex) { + s.orphanedOnSlot.OnUpdate(func(_, newValue iotago.SlotIndex) { callback(newValue) }) } diff --git a/pkg/protocol/engine/mempool/v1/mempool.go b/pkg/protocol/engine/mempool/v1/mempool.go index b5d06f484..84870661d 100644 --- a/pkg/protocol/engine/mempool/v1/mempool.go +++ b/pkg/protocol/engine/mempool/v1/mempool.go @@ -215,9 +215,7 @@ func (m *MemPool[VoteRank]) Evict(slot iotago.SlotIndex) { if delayedOutputs, exists := m.delayedOutputStateEviction.Get(delayedEvictionSlot); exists { delayedOutputs.ForEach(func(stateID iotago.Identifier, state *StateMetadata) bool { - if !m.cachedStateRequests.Delete(stateID, state.HasNoSpenders) && m.cachedStateRequests.Has(stateID) { - state.onAllSpendersRemoved(func() { m.cachedStateRequests.Delete(stateID, state.HasNoSpenders) }) - } + m.cachedStateRequests.Delete(stateID, state.HasNoSpenders) return true }) @@ -333,7 +331,7 @@ func (m *MemPool[VoteRank]) forkTransaction(transactionMetadata *TransactionMeta if err := m.conflictDAG.UpdateConflictingResources(transactionMetadata.ID(), resourceIDs); err != nil { // this is a hack, as with a reactive.Variable we cannot set it to 0 and still check if it was orphaned. - transactionMetadata.orphaned.Set(1) + transactionMetadata.orphanedOnSlot.Set(1) m.errorHandler(err) } diff --git a/pkg/protocol/engine/mempool/v1/state_metadata.go b/pkg/protocol/engine/mempool/v1/state_metadata.go index beae98396..23d3c6729 100644 --- a/pkg/protocol/engine/mempool/v1/state_metadata.go +++ b/pkg/protocol/engine/mempool/v1/state_metadata.go @@ -54,8 +54,8 @@ func (s *StateMetadata) setup(optSource ...*TransactionMetadata) *StateMetadata source.OnPending(func() { s.accepted.Set(false) }) source.OnAccepted(func() { s.accepted.Set(true) }) source.OnRejected(func() { s.rejected.Trigger() }) - source.OnCommitted(lo.Void(s.committed.Set)) - source.OnOrphaned(lo.Void(s.orphaned.Set)) + source.OnCommitted(lo.Void(s.committedOnSlot.Set)) + source.OnOrphaned(lo.Void(s.orphanedOnSlot.Set)) return s } diff --git a/pkg/protocol/engine/mempool/v1/transaction_metadata.go b/pkg/protocol/engine/mempool/v1/transaction_metadata.go index e84f5beab..1183e3223 100644 --- a/pkg/protocol/engine/mempool/v1/transaction_metadata.go +++ b/pkg/protocol/engine/mempool/v1/transaction_metadata.go @@ -214,8 +214,8 @@ func (t *TransactionMetadata) markInputSolid() (allInputsSolid bool) { return false } -func (t *TransactionMetadata) Commit(slot iotago.SlotIndex) { - t.committed.Set(slot) +func (t *TransactionMetadata) Commit() { + t.committedOnSlot.Set(t.earliestIncludedValidAttachment.Get().Slot()) } func (t *TransactionMetadata) IsConflicting() bool { @@ -251,7 +251,7 @@ func (t *TransactionMetadata) setupInput(input *StateMetadata) { input.OnRejected(func() { t.rejected.Trigger() }) input.OnOrphaned(func(slot iotago.SlotIndex) { - t.orphaned.Set(slot) + t.orphanedOnSlot.Set(slot) }) input.OnAccepted(func() { if atomic.AddUint64(&t.unacceptedInputsCount, ^uint64(0)) == 0 { @@ -279,7 +279,7 @@ func (t *TransactionMetadata) setupInput(input *StateMetadata) { input.OnSpendCommitted(func(spender mempool.TransactionMetadata) { if spender != t { spender.OnCommitted(func(slot iotago.SlotIndex) { - t.orphaned.Set(slot) + t.orphanedOnSlot.Set(slot) }) } }) @@ -295,8 +295,8 @@ func (t *TransactionMetadata) setup() (self *TransactionMetadata) { }) t.allValidAttachmentsEvicted.OnUpdate(func(_, slot iotago.SlotIndex) { - if !lo.Return2(t.IsCommitted()) { - t.orphaned.Set(slot) + if !lo.Return2(t.GetCommittedSlot()) { + t.orphanedOnSlot.Set(slot) } }) diff --git a/pkg/testsuite/transactions.go b/pkg/testsuite/transactions.go index 31585778b..3b2b3e2c5 100644 --- a/pkg/testsuite/transactions.go +++ b/pkg/testsuite/transactions.go @@ -126,7 +126,7 @@ func (t *TestSuite) AssertTransactionsInCachePending(expectedTransactions []*iot } func (t *TestSuite) AssertTransactionsInCacheOrphaned(expectedTransactions []*iotago.Transaction, expectedFlag bool, nodes ...*mock.Node) { - t.assertTransactionsInCacheWithFunc(expectedTransactions, expectedFlag, func(tm mempool.TransactionMetadata) bool { return lo.Return2(tm.IsOrphaned()) }, nodes...) + t.assertTransactionsInCacheWithFunc(expectedTransactions, expectedFlag, func(tm mempool.TransactionMetadata) bool { return lo.Return2(tm.GetOrphanedSlot()) }, nodes...) } func (t *TestSuite) AssertTransactionInCacheConflicts(transactionConflicts map[*iotago.Transaction][]string, nodes ...*mock.Node) {