Page MenuHomePhabricator

[Chronik] Add `lookup_spent_coins` and `uncache_coins` to `ChronikBridge`
ClosedPublic

Authored by tobias_ruck on Dec 30 2023, 20:26.

Details

Summary

This allows Chronik to use the already very optimized functions to access spent coins, which is useful when validating a raw tx, e.g. before broadcasting.

We also provide the coins that couldn't be found, and the coins to uncache if the tx didn't end up being broadcast.

This is so that clients can't fill our cache with useless old coins and slow down validation. This mirrors the behavoir of MemPoolAccept::PreChecks, which uncaches the queried coins if they don't end up being spent.

Test Plan

ninja check

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Tail of the build log:

   Compiling object v0.31.1
   Compiling cc v1.0.83
   Compiling backtrace v0.3.68
   Compiling stable-eyre v0.2.2
   Compiling abc-rust-error v0.1.0 (/work/chronik/abc-rust-error)
    Finished test [unoptimized + debuginfo] target(s) in 4m 24s
     Running unittests src/lib.rs (abc-ci-builds/build-chronik/cargo/build/debug/deps/abc_rust_error-a86f88dbd554c6db)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/test_error.rs (abc-ci-builds/build-chronik/cargo/build/debug/deps/test_error-d8569d21ff175837)

running 1 test
test test_error ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running unittests src/lib.rs (abc-ci-builds/build-chronik/cargo/build/debug/deps/abc_rust_lint-dba48c2d41985cd4)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

   Doc-tests abc-rust-error

running 1 test
test chronik/abc-rust-error/src/http_status.rs - http_status::parse_error_status (line 10) ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.25s

   Doc-tests abc-rust-lint

running 1 test
test chronik/abc-rust-lint/src/lib.rs - lint (line 13) ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.15s

[6/6] cd /work && /usr/bin/cmake -E env CARGO_TARGET_DIR="/work/abc-ci-builds/build-chronik/cargo/build" CARGO_BUILD_RUSTC="/root/.rustup/toolchains/1.72.0-x86_64-unknown-linux-gnu/bin/rustc" CARGO_BUILD_RUSTDOC="/root/.rustup/toolchains/1.72.0-x86_64-unknown-linux-gnu/bin/rustdoc" /root/.rustup/toolchains/1.72.0-x86_64-unknown-linux-gnu/bin/cargo --locked clippy --package abc-rust-* -- -D warnings
warning: some crates are on edition 2021 which defaults to `resolver = "2"`, but virtual workspaces default to `resolver = "1"`
note: to keep the current resolver, specify `workspace.resolver = "1"` in the workspace root's manifest
note: to use the edition 2021 resolver, specify `workspace.resolver = "2"` in the workspace root's manifest
    Blocking waiting for file lock on package cache
    Blocking waiting for file lock on package cache
    Blocking waiting for file lock on package cache
    Blocking waiting for file lock on package cache
    Blocking waiting for file lock on build directory
   Compiling libc v0.2.147
    Checking bytes v1.4.0
    Checking memchr v2.5.0
    Checking object v0.31.1
    Checking http v0.2.9
   Compiling cc v1.0.83
   Compiling backtrace v0.3.68
    Checking stable-eyre v0.2.2
    Checking abc-rust-error v0.1.0 (/work/chronik/abc-rust-error)
    Finished dev [unoptimized + debuginfo] target(s) in 4m 26s
ninja: build stopped: cannot make progress due to previous errors.
Build build-chronik failed with exit code 1

push the right branch history

Fabien requested changes to this revision.Jan 2 2024, 20:26
Fabien added inline comments.
chronik/chronik-bridge/src/ffi.rs
217 ↗(On Diff #43803)
chronik/chronik-cpp/chronik_bridge.cpp
223 ↗(On Diff #43803)

nit: you don't gain anything at using WITH_LOCK here as it covers the whole function, and it's adding an indentation level. Just use LOCK(cs_main)

227 ↗(On Diff #43803)
228–229 ↗(On Diff #43803)
240 ↗(On Diff #43803)

You can simplify a bit (like it's one in PreChecks) and unconditionally add to coins_to_uncache if !had_cached.
Since both Uncache() and HaveCoinInCache() cost a lookup it's less code for similar efficiency. You can even explain in a comment.

251 ↗(On Diff #43803)

dito

255–256 ↗(On Diff #43803)
This revision now requires changes to proceed.Jan 2 2024, 20:26
chronik/chronik-cpp/chronik_bridge.cpp
227 ↗(On Diff #43803)

actually we modify the input at the last line of the loop

240 ↗(On Diff #43803)

Makes sense—should I just remove or update the tests accordingly?

chronik/chronik-cpp/chronik_bridge.cpp
240 ↗(On Diff #43803)

Yes update the tests, it might also impact the uncache part of the test

change coins_to_uncache to possibly include mempool UTXOs, use LOCK(cs_main), make TxId in one line, fix comments

Fabien added inline comments.
chronik/chronik-cpp/chronik_bridge.cpp
221 ↗(On Diff #43899)

Nit: you can take the lock after you cleared the vectors

This revision is now accepted and ready to land.Jan 4 2024, 21:10