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

More tests for IndexedTxGraph and TxGraph #958

Closed
3 tasks
evanlinjin opened this issue Apr 28, 2023 · 12 comments
Closed
3 tasks

More tests for IndexedTxGraph and TxGraph #958

evanlinjin opened this issue Apr 28, 2023 · 12 comments
Assignees
Labels
Milestone

Comments

@evanlinjin
Copy link
Member

evanlinjin commented Apr 28, 2023

Describe the enhancement

Now that #926 is merged, let's look at having more fine-grained tests.

Please comment below if you wish to work on a test (to avoid duplicate work!).

Proposed tests

For TxGraph

  • Test behavior of inserting Anchors without the corresponding transaction existing in the graph. We should allow for this behavior.

    1. Insert various anchors for a non-existing txid. Ensure Additions are as expected.
    2. Merge the resultant Additions of the previous step, and attempt to recover a new TxGraph with them (this should succeed).
    3. Now, insert the actual transaction of txid. Ensure anchors still exist.
    4. Now, attempt recovery of the TxGraph again with the appended additions.
    • @remix75
  • Test TxGraph::get_chain_position and TxGraph::list_chain_txs behavior with unconfirmed conflicts. "Unconfirmed" can either mean that the txs do not have anchors, or that the anchors are not in the best chain. Both cases need to be tested. TxGraph should always pick the transaction with the latest last_seen value. What happens if the conflicts have the same last_seen value? - @rajarshimaitra

  • Test TxGraph::get_chain_position and TxGraph::list_chain_txs behavior with unconfirmed transactions (txs with no anchors) that conflict with transactions that are confirmed. The unconfirmed transactions should not be givne a chain position or be listed. - @LagginTimes

@rajarshimaitra
Copy link
Contributor

Test TxGraph::get_chain_position and TxGraph::list_chain_txs behavior with unconfirmed conflicts.

I will give this one a shot.

@remix7531
Copy link
Contributor

remix7531 commented Apr 29, 2023

Test behavior of inserting Anchors without the corresponding transaction existing in the graph. We should allow for this behavior.

I will work on this

@remix7531
Copy link
Contributor

Merge the resultant Additions of the previous step, and attempt to recover a new TxGraph with them

Now, attempt recovery of the TxGraph again with the appended additions.

What is meant by recovery in this context? We can insert things into a TxGraph and get information from it, but how is a TxGraph recovered?

@evanlinjin
Copy link
Member Author

evanlinjin commented May 3, 2023

What is meant by recovery in this context? We can insert things into a TxGraph and get information from it, but how is a TxGraph recovered?

When we mutate a TxGraph, tx_graph::Additions is returned. tx_graph::Additions can be Append::append-ed on top of one another to form an Additions that can be used to "recover" the `TxGraph:

let merged_additions = todo!("this is the appended additions from previous operations");

let mut graph = TxGraph::<AnchorImplementation>::default();
graph.apply_additions(merged_additions);

@LagginTimes
Copy link
Contributor

Test TxGraph::get_chain_position and TxGraph::list_chain_txs behavior with unconfirmed transactions (txs with no anchors) that conflict with transactions that are confirmed.

I will look into this one.

@rajarshimaitra
Copy link
Contributor

Hey @LagginTimes I already made a sketch for it. Have you progressed quite a lot? Happy to chat and sync up on this to see if we have covered all the cases. If your PR is closer to finish happy to hand it over to you and move on to other parts.

@evanlinjin
Copy link
Member Author

@rajarshimaitra No way have you started on that so quick.

@rajarshimaitra
Copy link
Contributor

@rajarshimaitra No way have you started on that so quick.

I included it in my PR as I was trying to cover all the cases with conflicts. PR soon.

@notmandatory notmandatory added this to BDK May 4, 2023
@notmandatory notmandatory removed this from BDK May 4, 2023
@notmandatory notmandatory added this to BDK May 4, 2023
@notmandatory notmandatory moved this to Todo in BDK May 4, 2023
@notmandatory notmandatory assigned evanlinjin and unassigned evanlinjin May 4, 2023
@notmandatory notmandatory assigned evanlinjin and unassigned evanlinjin May 6, 2023
@notmandatory notmandatory moved this from Todo to Needs Review in BDK May 9, 2023
remix7531 added a commit to remix7531/bdk that referenced this issue May 16, 2023
Test inserting anchors without the corresponding transaction
and testing the recoverability of the transaction graph

Partially fixes "More tests for IndexedTxGraph and TxGraph bitcoindevkit#958"
bitcoindevkit#958
@nondiremanuel
Copy link

@notmandatory shall we move this issue out of alpha.2 milestone or is it required?

@notmandatory
Copy link
Member

Let's check with @evanlinjin and team on Tuesday. It probably still needs to be done but could go to an alpha.3 with any other testing, hardening, documentation, and examples improvements.

@notmandatory
Copy link
Member

Replaced by #1063 will close when that one is completed.

@nondiremanuel
Copy link

I close this issue since #1063, which replaced it, was fixed by #1064

@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants