Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABCf1f03e5d4481: [Chronik] Add `Group` and `GroupHistoryReader/-Writer` to index the tx history…

Summary

A group can be "address", or "SLP token ID" or other protocols.

This struct allows us to conveniently and generically index the tx history of all such groups. The legacy Chronik indexer only allowed indexing by script/address, however, many people have been asking to add support to index by other predicates.

Since the tx history for some addresses is already +1M, we store the history paginated. This also allows APIs to more easily and efficiently serve paginated tx history.

Txs can be grouped by implementing Group on some struct, which then receives GroupQuerys, and returns Members, which are then grouped and sorted into the DB.

Depends on D13498.

Test Plan

ninja check-crates

Diff Detail

Repository

rABC Bitcoin ABC

Branch

chronik-group-txs

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 22812
Build 45244: Build Diff	build-chronik
Build 45243: arc lint + arc unit

Event Timeline

tobias_ruck created this revision.Mar 28 2023, 12:49

Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 28 2023, 12:49

tobias_ruck requested review of this revision.Mar 28 2023, 12:49

Harbormaster completed remote builds in B22790: Diff 38997.Mar 28 2023, 13:06

More questions that requesting changes, generally LGTM

chronik/chronik-bridge/src/ffi.rs
34 ↗	(On Diff #38997)	This change seems unrelated
chronik/chronik-db/src/group.rs
16 ↗	(On Diff #38997)	It would make sense to add time_first_seen to query as a time based group ?
chronik/chronik-db/src/io/group_history.rs
49 ↗	(On Diff #38997)	Actually sorting in descending order is probably more useful for the common case of displaying txs for an address, as you want to show the most recent txs first
117 ↗	(On Diff #38997)	Do you know about Duff's device ? No idea if that can be done with rust though (not requesting change here)
152 ↗	(On Diff #38997)	Can you explain why this is needed ? IIUC you need to check that there is no remaining removes when it completed page_num == 0, but the decrement should be a no-op since the loop will not run again ?
395 ↗	(On Diff #38997)
418 ↗	(On Diff #38997)	I don't understand this one, it looks like it's missing make_tx(4, [10], [40, 30, 40]), content

This revision now requires changes to proceed.Mar 28 2023, 20:58

tobias_ruck added inline comments.Mar 28 2023, 21:53

chronik/chronik-bridge/src/ffi.rs
34 ↗	(On Diff #38997)	I can add it in another diff.
chronik/chronik-db/src/group.rs
16 ↗	(On Diff #38997)	It's easy enough to add once needed, and having GroupQuery very much simplifies adding new stuff here. Nobody asked for querying by time yet though, so let's add it when needed.
chronik/chronik-db/src/io/group_history.rs
49 ↗	(On Diff #38997)	Yes. However, storing them in descending order is very tricky, as we have to shift all txs in all pages for every insert. In the ascending case, we just append the new txs in order to the latest page (or start new ones) and we're done. In case of a block delete, we just delete the last few txs of the last page (or delete the latest page and then some txs in the previous page) and we're done. Plus, we also need them in ascending order when a wallet tries to sync all tx history (e.g. when storing history locally, and only updating the latest txs). Plus, while the code to transform from ascending db pages to descending history pages is a little hard to wrap one's mind around, it's quite efficient and already written, so if we can make sure that's clean and documented I don't see a reason to store them descendingly.
117 ↗	(On Diff #38997)	this is the best we got: https://github.com/rust-lang/rust/blob/master/tests/ui/weird-exprs.rs
152 ↗	(On Diff #38997)	`page_num -= 1;` panics in debug mode if `page_num == 0`. Welcome to Rust! Also we'd do one unnecessary DB lookup of page 0xffffffff.
418 ↗	(On Diff #38997)	No, only txs 2 and 4 have a value=30 in there, and only txs 3 and 4 have a value=40. So this test makes sense.