Details

Reviewers

tobias_ruck

Group Reviewers

Restricted Project

Maniphest Tasks

Restricted Maniphest Task

Commits

rABCbf00137a339a: [chronik] add scripthash option to the various script endpoints

Summary

After this diff, all the existing script endpoints have a new script_type=scripthash option, except for the utxo scripthash endpoint will be dealt with in a separate diff, as it requires decompression of the script which is not trivial (decompressing p2pk scripts may require secp256k1)

A user can now fetch a script's history and utxo's by querying it's sha256 hash.

Depends on D16827

Test Plan

ninja check-functional

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 30905
Build 61313: Build Diff	chronik-client-integration-tests · ecash-lib-integration-tests · ecash-agora-integration-tests · build-without-wallet · build-diff · build-debug · build-clang-tidy · build-clang · build-chronik-plugins · build-chronik
Build 61312: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Fabien added a subscriber: Fabien.Oct 23 2024, 08:14

Fabien added inline comments.

test/functional/chronik_script_confirmed_txs.py
31 ↗	(On Diff #50279)	Just make it the default, if not on mainnet at least on regtest (add it to the regtest config file in util.py)

tobias_ruck requested changes to this revision.Oct 24 2024, 09:28

tobias_ruck added inline comments.

chronik/chronik-db/src/io/group_history.rs
224–225 ↗	(On Diff #50279)	doesn't this work? If not, try `Ok(Some(value.as_ref().to_vec()))`
260 ↗	(On Diff #50279)	make sure to harmonize this with the other diff, this is a bit confusing atm
592–595 ↗	(On Diff #50279)	No panics for normal DB failures
chronik/chronik-http/src/handlers.rs
111–118 ↗	(On Diff #50279)	this would be more idiomatic I suppose, but your solution is good too (in case my doesn't compile)
136 ↗	(On Diff #50279)	same here
156 ↗	(On Diff #50279)	I'm starting to think you should factor this into it's own function, maybe next to parse_script_hash. Return GroupMember<Script>. You then probably need to add a function `as_member_ref` turning `GroupMember<M>` into `GroupMember<&M>` (to make the borrow checker happy): something like this next to enum GroupMember impl<M> GroupMember<M> { pub fn as_member_ref(&self) -> GroupMember<&M> { match self { Member(member) => Member(member), MemberHash(hash) => MemberHash(*hash), } } } or maybe even implement `AsRef` for this, but might be difficult.
164 ↗	(On Diff #50279)	and then use it here (ideally using AsRef ofc)

This revision now requires changes to proceed.Oct 24 2024, 09:28

rebase, feedback

PiRK added inline comments.Oct 24 2024, 10:55

chronik/chronik-http/src/handlers.rs
156 ↗	(On Diff #50279)	Yeah I tried factoring the code into a function, but ran into this kind of script lifetime issue and GroupMember<M> vs GroupMember<&M> issue. Will try the suggested method.
test/functional/chronik_script_confirmed_txs.py
31 ↗	(On Diff #50279)	That would be the only optional index whose default enabled state is controlled by the framework's bitcoin.conf It feels weird.

Harbormaster completed remote builds in B30803: Diff 50373.Oct 24 2024, 11:27

tobias_ruck requested changes to this revision.Oct 24 2024, 12:24

tobias_ruck added inline comments.

chronik/chronik-db/src/group.rs
45 ↗	(On Diff #50373)	Btw, it seems ideomatic to just name this `as_ref`, see this for example: https://doc.rust-lang.org/src/core/result.rs.html#707 Does the same thing but no special name
chronik/chronik-indexer/src/query/group_history.rs
117 ↗	(On Diff #50373)	Pretty sure you can factor this into a method, something like fn member_ser_from_member(&self, member: &GroupMember<G::Member<'_>>) -> Result<Bytes> { ... }

This revision now requires changes to proceed.Oct 24 2024, 12:24

as_member_ref -> as_ref, factor repeated code into a member_ser_from_member method

Harbormaster completed remote builds in B30812: Diff 50395.Oct 24 2024, 16:03

Build Bitcoin ABC Diffs / Diff Testing (build-chronik) failed.

Tail of the build log:

   Compiling prost-build v0.11.9
    Checking chronik-plugin-common v0.1.0 (/work/chronik/chronik-plugin-common)
    Checking utf-8 v0.7.6
    Checking embedded-io v0.4.0
    Checking ryu v1.0.17
    Checking smallvec v1.13.2
    Checking postcard v1.0.8
    Checking chronik-plugin v0.1.0 (/work/chronik/chronik-plugin)
    Checking tungstenite v0.21.0
    Checking hyper v1.2.0
    Checking pin-project v1.1.5
    Checking serde_spanned v0.6.6
    Checking toml_datetime v0.6.6
   Compiling pyo3 v0.22.2
    Checking mime v0.3.17
    Checking winnow v0.6.13
    Checking topo_sort v0.4.0
    Checking sync_wrapper v0.1.2
    Checking seahash v4.1.0
    Checking axum-core v0.4.3
    Checking tokio-tungstenite v0.21.0
    Checking tower v0.4.13
    Checking hyper-util v0.1.3
    Checking memoffset v0.9.1
    Checking serde_json v1.0.115
    Checking serde_urlencoded v0.7.1
    Checking futures-executor v0.3.30
    Checking serde_path_to_error v0.1.16
    Checking base64 v0.21.7
    Checking unicode-segmentation v1.11.0
   Compiling chronik-proto v0.1.0 (/work/chronik/chronik-proto)
    Checking matchit v0.7.3
    Checking unindent v0.2.3
    Checking sync_wrapper v1.0.1
    Checking tower-http v0.5.2
    Checking futures v0.3.30
    Checking convert_case v0.6.0
   Compiling chronik-bridge v0.1.0 (/work/chronik/chronik-bridge)
    Checking toml_edit v0.22.14
   Compiling librocksdb-sys v0.11.0+8.1.1
    Checking axum v0.7.5
   Compiling pyo3-macros v0.22.2
    Checking toml v0.8.14
    Checking chronik-plugin-impl v0.1.0 (/work/chronik/chronik-plugin-impl)
    Checking rocksdb v0.21.0
    Checking chronik-db v0.1.0 (/work/chronik/chronik-db)
    Checking chronik-indexer v0.1.0 (/work/chronik/chronik-indexer)
error: this expression creates a reference which is immediately dereferenced by the compiler
   --> chronik/chronik-indexer/src/query/group_history.rs:107:39
    |
107 |                 self.group.ser_member(&member).as_ref(),
    |                                       ^^^^^^^ help: change this to: `member`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
    = note: `-D clippy::needless-borrow` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::needless_borrow)]`

error: could not compile `chronik-indexer` (lib) due to 1 previous error
ninja: build stopped: cannot make progress due to previous errors.
Build build-chronik failed with exit code 1

Build Bitcoin ABC Diffs / Diff Testing (build-chronik-plugins) failed.

Tail of the build log:

...  |
19 | |     mod test;
20 | | }
   | |_^
   = note: `#[warn(unused_imports)]` implied by `#[warn(unused)]`
   = note: this warning originates in the macro `abc_rust_lint::lint` (in Nightly builds, run with -Z macro-backtrace for more info)

warning: `chronik-db` (lib test) generated 1 warning (run `cargo fix --lib -p chronik-db --tests` to apply 1 suggestion)
    Finished test [unoptimized + debuginfo] target(s) in 6m 32s
     Running unittests src/lib.rs (abc-ci-builds/build-chronik-plugins/cargo/build/debug/deps/chronik_db-a3c26c153718321c)

running 37 tests
test groups::script::tests::test_script_group ... ok
test index_tx::tests::test_tx_num_cache ... ok
test io::block_stats::tests::test_block_stats ... ok
test io::group_utxos::tests::test_value_group_utxos ... ok
test io::token::tests::test_batch_common::test_batch_skip_validation ... ok
test io::group_history::tests::test_value_group_history ... ok
test io::token::tests::test_batch_common::test_batch_cycle ... ok
test io::token::tests::test_batch_common::test_batch_topological_sort ... ok
test io::token::tests::test_batch_burn::test_batch_burn ... ok
test io::token::tests::test_batch_alp::test_batch_alp ... ok
test mem::group_history::tests::test_mempool_group_history ... ok
test mem::group_utxos::tests::test_mempool_group_utxos ... ok
test mem::spent_by::tests::test_mempool_spent_by ... ok
test io::token::tests::test_batch_genesis::test_batch_genesis_alp ... ok
test io::token::tests::test_batch_disconnect_block::test_batch_disconnect ... ok
test io::spent_by::tests::test_spent_by ... ok
test io::token::tests::test_batch_genesis::test_batch_genesis_slp_fungible ... ok
test io::token::tests::test_batch_vault::test_batch_vault ... ok
test ser::tests::test_deserialize_err ... ok
test ser::tests::test_deserialize_leftover_err ... ok
test ser::tests::test_err_display_deserialize ... ok
test ser::tests::test_err_display_deserialize_leftover ... ok
test ser::tests::test_err_display_serialize ... ok
test ser::tests::test_roundtrip ... ok
test ser::tests::test_roundtrip_vec ... ok
test ser::tests::test_serialize_err ... ok
test io::token::tests::test_batch_unknown::test_batch_unknown ... ok
test io::token::tests::test_batch_nft::test_batch_slp_nft1 ... ok
test mem::tokens::tests::test_mempool_tokens ... ok
test io::merge::tests::test_catch_merge ... ok
test reverse_lookup::tests::test_reverse_lookup ... ok
test plugins::io::tests::test_plugin_metas ... ok
test plugins::io::tests::test_plugin_writer ... ok
test io::blocks::tests::test_blocks ... ok
test index_tx::tests::test_prepare_indexed_txs ... ok
test io::txs::tests::test_insert_txs ... ok
test reverse_lookup::tests::test_reverse_lookup_rng ... ok

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

   Doc-tests chronik-db

running 0 tests

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

ninja: build stopped: cannot make progress due to previous errors.
Build build-chronik-plugins failed with exit code 1

PiRK planned changes to this revision.Oct 25 2024, 06:56

fix a warning

Harbormaster completed remote builds in B30833: Diff 50426.Oct 25 2024, 08:41

PiRK mentioned this in rABC8cabfa5c8575: [chronik] store a scripthash -> script index.Oct 25 2024, 08:59

tobias_ruck requested changes to this revision.Oct 26 2024, 11:59

tobias_ruck added inline comments.

test/functional/chronik_script_confirmed_txs.py
1 ↗	(On Diff #50426)	Tbh I think it makes more sense to test this in its own test file chronik_script_hash_index Then leave these tests untouched; especially the script history test is really complicated already Also test enabling / disabling (wiping) the index, like I did for the lokad ID index We don’t need to test pagination with the script hash index
31 ↗	(On Diff #50426)	Then we can also remove this
81 ↗	(On Diff #50426)	Maybe add some more cases, eg empty string, odd length, off by one, non-hex etc

This revision now requires changes to proceed.Oct 26 2024, 11:59

write a new test dedicated to scripthash, make it a bit simpler (no need to retest all the things already tested in the other 3 tests)

Harbormaster completed remote builds in B30869: Diff 50480.Oct 28 2024, 13:28

tobias_ruck requested changes to this revision.Oct 29 2024, 11:30

tobias_ruck added inline comments.

chronik/chronik-db/src/io/group_history.rs
221 ↗	(On Diff #50480)	better to return an error here instead of panicing you have to add it to the derive(Error) enum above
test/functional/chronik_scripthash.py
174 ↗	(On Diff #50480)	Do this in this diff, just return an Err with a `400:` prefixed (see other errors like that for reference) explaining that the scripthash index is disabled

This revision now requires changes to proceed.Oct 29 2024, 11:30

PiRK added a child revision: D17037: [chronik] index scripts by scripthashes in the mempool.Oct 29 2024, 14:08

PiRK added inline comments.Oct 29 2024, 14:54

test/functional/chronik_scripthash.py
174 ↗	(On Diff #50480)	It is really not trivial at the moment to find out whether the db does not have the scripthast because the index was not enabled or because thus hash is just missing from the db. I see a simple solution of reading FIELD_SCRIPTHASH_INDEX_ENABLED in the meta cf, but that means two db accesses for each scripthash fetch which is likely not optimal (unless rocksdb is smart enough to cache the frequently access field in memory?)

don't panic

Harbormaster completed remote builds in B30903: Diff 50528.Oct 29 2024, 15:17

Build Bitcoin ABC Diffs / Diff Testing (build-chronik) failed.

Tail of the build log:

   Doc-tests chronik-bridge

running 0 tests

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

   Doc-tests chronik-db

running 0 tests

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

   Doc-tests chronik-http

running 1 test
test chronik/chronik-http/src/protobuf.rs - protobuf::Protobuf (line 29) ... ok

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

   Doc-tests chronik-indexer

running 0 tests

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

   Doc-tests chronik-plugin

running 0 tests

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

   Doc-tests chronik-plugin-common

running 0 tests

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

   Doc-tests chronik-plugin-impl

running 0 tests

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

   Doc-tests chronik-proto

running 0 tests

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

   Doc-tests chronik-util

running 2 tests
test chronik/chronik-util/src/log.rs - log::log (line 65) ... ignored
test chronik/chronik-util/src/log.rs - log::log_chronik (line 87) ... ignored

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

ninja: build stopped: cannot make progress due to previous errors.
Build build-chronik failed with exit code 1

Build Bitcoin ABC Diffs / Diff Testing (build-chronik-plugins) failed.

Tail of the build log:

   Doc-tests chronik-bridge

running 0 tests

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

   Doc-tests chronik-db

running 0 tests

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

   Doc-tests chronik-http

running 1 test
test chronik/chronik-http/src/protobuf.rs - protobuf::Protobuf (line 29) ... ok

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

   Doc-tests chronik-indexer

running 0 tests

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

   Doc-tests chronik-plugin

running 0 tests

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

   Doc-tests chronik-plugin-common

running 0 tests

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

   Doc-tests chronik-plugin-impl

running 0 tests

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

   Doc-tests chronik-proto

running 0 tests

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

   Doc-tests chronik-util

running 2 tests
test chronik/chronik-util/src/log.rs - log::log (line 65) ... ignored
test chronik/chronik-util/src/log.rs - log::log_chronik (line 87) ... ignored

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

ninja: build stopped: cannot make progress due to previous errors.
Build build-chronik-plugins failed with exit code 1

use ok_or, fix the test suite's name

Harbormaster completed remote builds in B30905: Diff 50530.Oct 29 2024, 16:27

PiRK mentioned this in D17043: [chronik] implement scripthash type for utxos endpoints.Oct 30 2024, 09:25

tobias_ruck requested changes to this revision.Oct 30 2024, 16:39

tobias_ruck added inline comments.

chronik/chronik-db/src/io/group_history.rs
132
227
test/functional/chronik_scripthash.py
174	don't forget about this one 🙃

This revision now requires changes to proceed.Oct 30 2024, 16:39

return a better error if the scripthash index is disabled

Harbormaster completed remote builds in B30939: Diff 50575.Oct 31 2024, 12:29

use then_some, don't allocate to query the hashmap

I accidentaly squashed the child diff while trying to reabase and amend it

revert accidental squashing

no more need for the todo

Harbormaster completed remote builds in B30941: Diff 50580.Oct 31 2024, 14:07

Harbormaster completed remote builds in B30942: Diff 50581.Oct 31 2024, 14:25

Harbormaster completed remote builds in B30944: Diff 50583.Oct 31 2024, 14:56

remove the need to access db for checking if the index is enabled

remove now useless functions and imports

Harbormaster completed remote builds in B30945: Diff 50584.Oct 31 2024, 16:58

Harbormaster completed remote builds in B30946: Diff 50585.Oct 31 2024, 17:27

One nit

chronik/chronik-db/src/io/group_history.rs
223 ↗	(On Diff #50575)	Let's not duplicate this from MetadataReader but just use it
chronik/chronik-http/src/handlers.rs
6 ↗	(On Diff #50585)	Merge those two imports in one big one bitcoinsuite_core::{…}