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 conflicting tx situations. #964

Closed

Conversation

rajarshimaitra
Copy link
Contributor

Description

partial fixes #958 . These tests attempt to cover all the conflicting scenarios.

The following tests are included:

  • Mempool conflicts are chosen by the latest last seen.
  • Conflicts in mempool and orphaned block filters out the orphaned block tx.
  • Conflicts with mempool and confirmed tx, filters out mempool.
  • Conflicts with two mempool tx at same seen_at.

Some helper functions in the common module.

Notes to the reviewers

As noted by @evanlinjin on the test description.

What happens if the conflicts have the same last_seen value?

A possible solution for this situation is mentioned in the comment of the last test, which covers this case. I will attempt a PR to enforce the behavior if that is a reasonable approach.

Changelog notice

Checklists

All Submissions:

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

@notmandatory
Copy link
Member

notmandatory commented May 5, 2023

Looks like this one needs a cargo fmt .. otherwise is it ready to review or dependent on #963 ?

@evanlinjin
Copy link
Member

Looks like this one needs a cargo fmt .. otherwise is it ready to review or dependent on #963 ?

No dependencies, I saw that CI wasn't passing so didn't review.

The following tests are included

 - Mempool conflicts are chosen by latest last seen.
 - Conflicts in mempool and orphaned block, filters out the orphaned block
tx.
 - Conflicts with mempool and confirmed tx, filters out mempool.
 - Conflicts with two mempool tx at same `seen_at`. This currently lists
 both the txs. A potential fix in mentioned in the comments.

 Some helper functions in the common module.
@rajarshimaitra
Copy link
Contributor Author

rajarshimaitra commented May 5, 2023

My bad. Had the fmt fix, but forgot to push. This had a dependency on #963 as the Anchor type changed. Fixed now..

@evanlinjin
Copy link
Member

Will look at this first thing tomorrow.

Comment on lines +506 to +510
// We only have one utxo. The latest `tx_conflict_2`.
assert_eq!(
utxos.pop().unwrap().chain_position,
ObservedAs::Unconfirmed(200)
);
Copy link
Member

Choose a reason for hiding this comment

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

Why could we not assert all of utxo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can, but will make the tests code too verbose. The point here is to test various txout positions. If we wanna test the whole structure of FullTxOut, could be done in a separate test.

@evanlinjin
Copy link
Member

@rajarshimaitra as we discussed.

struct TxTemplate<'a, K> {
    pub tx_name: &'a str,
    pub spends: &'a [(&'a str, usize)], // (tx_name, vout)
    pub outputs: &'a [TxOutTemplate<K>],
    pub anchors: &'a [(u32, BlockId)],
    pub last_seen: Option<u64>,
}

struct TxOutTemplate<K> {
    pub value: u64,
    pub owned_spk: Option<(K, u32)>, // some = derive a spk from K, none = random spk
}

fn init_graph<'a, K: 'a>(
    descriptors: HashMap<K, Descriptor<DescriptorPublicKey>>,
    tx_templates: impl IntoIterator<Item = TxTemplate<'a, K>>,
) -> (
    IndexedTxGraph<ConfirmationHeightAnchor, KeychainTxOutIndex<K>>,
    HashMap<&'a str, Txid>,
) {
    todo!()
}

#[test]
fn example_test() {
    struct TestCase<'a, K> {
        tx_templates: &'a [TxTemplate<'a, K>],
        chain_tip: BlockId,
        expected_list_chain_txs: (),
    }
}

@evanlinjin
Copy link
Member

@LagginTimes Could you take over on this?

@LagginTimes
Copy link
Contributor

@LagginTimes Could you take over on this?

Sure, I can start looking into this.

@notmandatory
Copy link
Member

@LagginTimes any updates on this? should I keep it in the alpha.1 milestone?

@notmandatory notmandatory removed this from the 1.0.0-alpha.1 milestone Jun 20, 2023
@evanlinjin
Copy link
Member

Replaced by #1064

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

More tests for IndexedTxGraph and TxGraph
4 participants