Store txConflicting reason correctly and rename ConflictDAG to SpendDAG #508

jkrvivian · 2023-11-08T13:40:48Z

Remove optForkAllTransactions option. The default is to fork all transactions.
Store conflicting transaction failure reason on TransactionMetadata OnRejected event.
Rename ConflictDAG to SpendDAG.
Since we fork every transaction, not just conflicts are included.
Rename Conflict to Spender and ConflictID to SpenderID because these objects represent transactions (transactions are generically referred to as a spender, and an output is generically referred to as a resource).

Close #462

…correctly

piotrm50

Please run a search through the whole codebase again as there seems to be a lot conflicts left.

components/dashboard/type.go

pkg/protocol/engine/mempool/spenddag/tests/assertions.go

pkg/protocol/engine/mempool/spenddag/events.go

pkg/protocol/engine/mempool/spenddag/spenddagv1/spenddag.go

pkg/protocol/engine/booker/inmemorybooker/booker.go

cyberphysic4l · 2023-11-20T12:55:47Z

A spendable resource is identified by an iotago.OutputID, we don't need a generic ResourceID just for the spend DAG because we are always talking about about utxos which are just iotago.Output . We should drop the generic "resource" because it's not actually generic in how we use it so it is just unhelpful. resources should be referred to as utxos.
A Transaction contains one or more spends, each of different outputs. A spend can be therefore be identified by a tuple (iotago.TransactionID, iotago.OutputID) . What we previously called SpendID or ConflictID was actually just a iotago.TransactionID . This is incorrect because a spend != transaction, it is a spend of a specific utxo within a transaction, so it needs a transaction ID and output ID to identify it uniquely. We should get rid of ConflictID/SpendID altogether.
What we previously called a ConflictSet should now be a called a SpendSet . Each SpendSet corresponds to a single utxo. If the SpendSet contains more than one spend for that utxo, then we say that it contains a conflict. There will be no object called a conflict anymore, but a conflict can be identified by a spend set containing more than one spend or by a spend with a set of ConflictingSpends associated to it that is not empty.

piotrm50

Another batch of missed conflict renames.

piotrm50 · 2023-11-21T08:18:25Z

components/dashboard/frontend/src/app/components/ExplorerAddressResult.tsx

@@ -222,10 +222,10 @@ class OutputMeta extends React.Component<omProps, any> {
        let pendingMana = this.props.pendingMana;
        return (
            <ListGroup>
-                ConflictIDs: 
+                spendIDs: 


Missing capital letter in Spend here and in other .tsx files.

Why was that resolved without a comment? Because I also don't get the change...

pkg/protocol/engine/mempool/spenddag/spenddagv1/spend_set.go

pkg/protocol/engine/mempool/spenddag/tests/assertions.go

pkg/protocol/engine/mempool/spenddag/tests/tests.go

pkg/protocol/engine/mempool/tests/tests.go

pkg/protocol/engine/mempool/spenddag/spenddagv1/spenddag_test.go

muXxer

This is such a massive "renaming PR".... I don't like that it also includes logic changes at the same time.

For the next time you do such a refactor, it would be much better to split it up into several PRs.

For example, use the rename capability of the IDE only, no "find & replace" (I see that you used that because uncommented code also changed). And then create commits per single "rename". This is much easier to check, because we all can simply trust your IDE.

The logic changes should be part of another PR next time.

Regarding the new naming and the logic, I totally agree with @cyberphysic4l 's comment.
Maybe we should create another type for the tuple of (iotago.TransactionID, iotago.OutputID) in another PR and fix the logic.

I also think the usage of "transactionID" instead of "spendID" in some places makes more sense.

Should we merge this PR or will this heavily conflict with the react branches?

muXxer · 2023-11-22T18:59:35Z

components/dashboard/frontend/src/app/components/ExplorerAddressResult.tsx

@@ -222,10 +222,10 @@ class OutputMeta extends React.Component<omProps, any> {
        let pendingMana = this.props.pendingMana;
        return (
            <ListGroup>
-                ConflictIDs: 
+                spendIDs: 


Why was that resolved without a comment? Because I also don't get the change...

components/dashboard/frontend/src/app/components/ExplorerBlockQueryResult.tsx

components/dashboard/frontend/src/app/components/ExplorerOutputQueryResult.tsx

components/dashboard/frontend/src/app/stores/ExplorerStore.tsx

jkrvivian added 6 commits November 7, 2023 14:09

Remove optForkAllTransactions

3c631d2

Store conflictTx failure reason on rejected txs

4063dc2

Rename ConflictDAG to SpendDAG

f5a30c4

Merge branch 'develop' into fix/conflicting-tx-reason

9280eb4

Merge branch 'develop' into fix/conflicting-tx-reason

e01553a

Hook to TransactionMetadata OnRejected to capture conflicting reason …

02fb2f7

…correctly

jkrvivian requested a review from karimodm November 8, 2023 13:40

jkrvivian added 5 commits November 9, 2023 10:15

Merge branch 'develop' into fix/conflicting-tx-reason

8561076

Remove TestAllWithoutForkingEverything

e6dd308

Remove TestAllWithoutForkingEverything

2500e79

Rename variable spenddag to spendDAG

0c78f57

Rename conflict to spend

2857d11

jkrvivian force-pushed the fix/conflicting-tx-reason branch from 64bf4f1 to 2857d11 Compare November 9, 2023 07:43

jkrvivian requested a review from piotrm50 November 9, 2023 08:14

Fix resource cleanup unit test to work with forking all transactions.

2e2f241

piotrm50 requested changes Nov 13, 2023

View reviewed changes

jkrvivian added 3 commits November 15, 2023 16:44

Resolve comments

e6f7360

Merge branch 'develop' into fix/conflicting-tx-reason

5f64dfb

Fix unit test

c81bc25

jkrvivian requested a review from piotrm50 November 15, 2023 09:12

cyberphysic4l reviewed Nov 20, 2023

View reviewed changes

pkg/protocol/engine/booker/inmemorybooker/booker.go Outdated Show resolved Hide resolved

Rename ConflictSet to SpendSet

f63865c

piotrm50 requested changes Nov 21, 2023

View reviewed changes

jkrvivian added 2 commits November 21, 2023 16:44

Merge branch 'develop' into fix/conflicting-tx-reason

9e667b2

Rename conflict to spend in dashboard frontend

aa045a2

jkrvivian force-pushed the fix/conflicting-tx-reason branch from 23fb3ff to aa045a2 Compare November 21, 2023 11:57

cyberphysic4l added 3 commits November 21, 2023 15:16

spend -> spender

311b2ef

fix merge errors

62e675e

rename files

316a51e

cyberphysic4l and others added 2 commits November 22, 2023 10:51

remove broken line from spendDag test and clean up

357a1cd

Merge branch 'develop' into fix/conflicting-tx-reason

71709d1

muXxer reviewed Nov 22, 2023

View reviewed changes

Remove unused Spend related frontend

bf7c619

muXxer approved these changes Nov 27, 2023

View reviewed changes

jkrvivian added 2 commits November 27, 2023 20:41

Merge branch 'develop' into fix/conflicting-tx-reason

07d4330

Merge branch 'develop' into fix/conflicting-tx-reason

bd8b14c

jkrvivian requested a review from piotrm50 November 29, 2023 09:29

piotrm50 approved these changes Nov 29, 2023

View reviewed changes

Merge branch 'develop' into fix/conflicting-tx-reason

34d7e00

jkrvivian merged commit 26441e4 into develop Nov 29, 2023
4 checks passed

jkrvivian deleted the fix/conflicting-tx-reason branch November 29, 2023 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store txConflicting reason correctly and rename ConflictDAG to SpendDAG #508

Store txConflicting reason correctly and rename ConflictDAG to SpendDAG #508

jkrvivian commented Nov 8, 2023 •

edited by cyberphysic4l

Loading

piotrm50 left a comment

cyberphysic4l commented Nov 20, 2023 •

edited

Loading

piotrm50 left a comment

piotrm50 Nov 21, 2023

muXxer Nov 22, 2023

muXxer left a comment •

edited

Loading

muXxer Nov 22, 2023

Store txConflicting reason correctly and rename ConflictDAG to SpendDAG #508

Store txConflicting reason correctly and rename ConflictDAG to SpendDAG #508

Conversation

jkrvivian commented Nov 8, 2023 • edited by cyberphysic4l Loading

piotrm50 left a comment

Choose a reason for hiding this comment

cyberphysic4l commented Nov 20, 2023 • edited Loading

piotrm50 left a comment

Choose a reason for hiding this comment

piotrm50 Nov 21, 2023

Choose a reason for hiding this comment

muXxer Nov 22, 2023

Choose a reason for hiding this comment

muXxer left a comment • edited Loading

Choose a reason for hiding this comment

muXxer Nov 22, 2023

Choose a reason for hiding this comment

jkrvivian commented Nov 8, 2023 •

edited by cyberphysic4l

Loading

cyberphysic4l commented Nov 20, 2023 •

edited

Loading

muXxer left a comment •

edited

Loading