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

Temporary PR: Evicted at #1839

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

evanlinjin
Copy link
Member

@evanlinjin evanlinjin commented Feb 16, 2025

Description

This is here to make sure CI passes. These changes will be force-pushed to #1811 eventually.

Notes to the reviewers

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

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

In electrum, heights returned alongside txs may be 0 or -1. 0 means the
tx is unconfirmed. -1 means the tx is unconfirmed and spends an
unconfirmed tx.

Previously, the codebase assumed that heights cannot be negative and
used a `i32 as usize` cast (which would lead to panic if the i32 is
negative).
Is an spk history contains a tx that spends another unconfirmed tx, the
Electrum API will return the tx with a negative height. This should
succeed and not panic.
This may, if we introduce new fields to `TxUdpate`, they can be minor
non-breaking updates.
* Change `TxUpdate::seen_ats` to be a `HashSet<(Txid, u64)>` so we can
  introduce multiple timestamps per tx. This is useful to introduce both
  `first_seen` and `last_seen` timestamps to `TxGraph`. This is also a
  better API for chain-sources as they can just insert timestamps into
  the field without checking previous values.
* Change sync/full-scan flow to have the request structure introduce the
  sync time instead of introducing the timestamp when apply the
  `TxUpdate`. This simplifies the `apply_update{_at}` logic and makes
  `evicted_at` easier to reason about (in the future).
This is a set of evicted txs from the mempool.
evanlinjin and others added 3 commits February 16, 2025 22:41
The evicted-at and last-evicted timestamp informs the `TxGraph` when the
transaction was last deemed as missing (evicted) from the mempool.

The canonicalization algorithm is changed to disregard transactions with
a last-evicted timestamp greater or equal to it's last-seen timestamp.
The exception is when we have a canonical descendant due to rules of
transitivity.

Update rusqlite_impl to persist `last_evicted`.

Also update docs:
* Remove duplicate paragraphs about `ChangeSet`s.
* Add "Canonicalization" section which expands on methods that require
  canonicalization and the associated data types used in the
  canonicalization algorithm.
The spk history returned from Electrum should have these txs present.
Any missing tx will be considered evicted from the mempool.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants