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
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
Copy link
Author

pluveto commented Nov 25, 2024

Now it's rebased.

@LagginTimes
Copy link
Contributor

ACK 8374f70

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.

4 participants