Skip to content
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

test(chain): add several basic scenarios to test transaction conflicts #1739

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pluveto
Copy link

@pluveto pluveto commented Nov 21, 2024

Description

This update introduces several test cases to validate the transaction conflict handling logic within the TxGraph. The tests encompass a range of scenarios including:

  1. Simple cases with isolated transactions (only A exists).
  2. Scenarios with conflicting transactions (B and B' spending A) under two basic conditions:
    • One of the conflicting transactions is confirmed.
    • Conflicts are resolved based on the last_seen timestamp.

Changelog Notice

  • Added unit tests for transaction conflict resolution in TxGraph.

Checklists

All Submissions:

  • I've signed all my commits.
  • I followed the contribution guidelines.
  • I ran cargo fmt and cargo clippy before committing.

@LagginTimes
Copy link
Contributor

Thank you for your contribution! The test case involving two conflicting transactions with different last_seen values is already addressed by the existing test scenario: "2 unconfirmed txs with same last_seens conflict." Other than that, the rest of the tests look good to me.

@pluveto
Copy link
Author

pluveto commented Nov 24, 2024

OK. I've removed this redundant case.

@LagginTimes
Copy link
Contributor

Could you please squash the second commit into the first one to keep the commit history clean? Everything will look good to me once that's done. Thanks!

@pluveto pluveto force-pushed the test/chain-basic-cases branch from ba50fd7 to 8bda75d Compare November 25, 2024 03:45
@pluveto
Copy link
Author

pluveto commented Nov 25, 2024

Squash done.

@evanlinjin
Copy link
Member

@pluveto could you please rebase on master to fix CI? We just merged the fix for CI.

@pluveto pluveto force-pushed the test/chain-basic-cases branch from 7ad264a to 8374f70 Compare November 25, 2024 06:56
@pluveto
Copy link
Author

pluveto commented Nov 25, 2024

Now it's rebased.

@LagginTimes
Copy link
Contributor

ACK 8374f70

Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 8374f70

crates/chain/tests/test_tx_graph_conflicts.rs Outdated Show resolved Hide resolved
@pluveto pluveto force-pushed the test/chain-basic-cases branch from 8374f70 to 868739f Compare November 26, 2024 02:27
@pluveto pluveto force-pushed the test/chain-basic-cases branch from 868739f to 7c3f915 Compare November 26, 2024 14:01
@LagginTimes
Copy link
Contributor

ACK 7c3f915

Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 7c3f915

Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In reality, this should really fail. A transaction that doesn't have a last-seen nor anchor should not be part of the best chain.

The culprit is the TxTemplate code (crates/chain/tests/common/tx_template.rs) which is wrong. I've fixed it in #1670.

Would you be able to rebase this on #1670?

@pluveto pluveto force-pushed the test/chain-basic-cases branch from 7c3f915 to 1cd0b4f Compare December 3, 2024 13:31
@pluveto
Copy link
Author

pluveto commented Dec 3, 2024

Rebased

Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 1cd0b4f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

5 participants