Page MenuHomePhabricator
Differential D16937

[chronik] add scripthash option to the various script endpoints
ClosedPublic
Actions

Authored by PiRK on Oct 19 2024, 15:48.

Details

Reviewers
tobias_ruck
Group Reviewers
Restricted Project
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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
PiRK added a parent revision: D16827: [chronik] store a scripthash -> script index.Tue, Oct 22, 17:43
Fabien added a subscriber: Fabien.Wed, Oct 23, 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.Thu, Oct 24, 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.Thu, Oct 24, 09:28
PiRK updated this revision to Diff 50373.Thu, Oct 24, 10:52
PiRK marked an inline comment as done.

rebase, feedback

PiRK added inline comments.Thu, Oct 24, 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.Thu, Oct 24, 11:27
tobias_ruck requested changes to this revision.Thu, Oct 24, 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.Thu, Oct 24, 12:24
PiRK updated this revision to Diff 50395.Thu, Oct 24, 15:56

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

Harbormaster completed remote builds in B30812: Diff 50395.Thu, Oct 24, 16:03
teamcity added a comment.Thu, Oct 24, 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
teamcity added a comment.Thu, Oct 24, 16:05
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.Fri, Oct 25, 06:56
PiRK updated this revision to Diff 50426.Fri, Oct 25, 08:16

fix a warning

Harbormaster completed remote builds in B30833: Diff 50426.Fri, Oct 25, 08:41
PiRK mentioned this in rABC8cabfa5c8575: [chronik] store a scripthash -> script index.Fri, Oct 25, 08:59
tobias_ruck requested changes to this revision.Sat, Oct 26, 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.Sat, Oct 26, 11:59
PiRK updated this revision to Diff 50480.Mon, Oct 28, 13:03

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.Mon, Oct 28, 13:28
tobias_ruck requested changes to this revision.Tue, Oct 29, 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.Tue, Oct 29, 11:30
PiRK added a child revision: D17037: [chronik] index scripts by scripthashes in the mempool.Tue, Oct 29, 14:08
PiRK added inline comments.Tue, Oct 29, 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?)

PiRK updated this revision to Diff 50528.Tue, Oct 29, 14:56

don't panic

Harbormaster completed remote builds in B30903: Diff 50528.Tue, Oct 29, 15:17
teamcity added a comment.Tue, Oct 29, 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
teamcity added a comment.Tue, Oct 29, 15:19
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
PiRK updated this revision to Diff 50530.Tue, Oct 29, 15:40

use ok_or, fix the test suite's name

Harbormaster completed remote builds in B30905: Diff 50530.Tue, Oct 29, 16:27
PiRK mentioned this in D17043: [chronik] implement scripthash type for utxos endpoints.Wed, Oct 30, 09:25
tobias_ruck requested changes to this revision.Wed, Oct 30, 16:39
tobias_ruck added inline comments.
chronik/chronik-db/src/io/group_history.rs
132 ↗(On Diff #50530)
227 ↗(On Diff #50530)
test/functional/chronik_scripthash.py
174 ↗(On Diff #50530)

don't forget about this one 🙃

This revision now requires changes to proceed.Wed, Oct 30, 16:39
PiRK updated this revision to Diff 50575.Thu, Oct 31, 11:54

return a better error if the scripthash index is disabled

Harbormaster completed remote builds in B30939: Diff 50575.Thu, Oct 31, 12:29
PiRK updated this revision to Diff 50580.Thu, Oct 31, 13:28

use then_some, don't allocate to query the hashmap

PiRK planned changes to this revision.Thu, Oct 31, 13:31

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

PiRK updated this revision to Diff 50581.Thu, Oct 31, 13:43

revert accidental squashing

PiRK updated this revision to Diff 50583.Thu, Oct 31, 13:49

no more need for the todo

Harbormaster completed remote builds in B30941: Diff 50580.Thu, Oct 31, 14:07
Harbormaster completed remote builds in B30942: Diff 50581.Thu, Oct 31, 14:25
Harbormaster completed remote builds in B30944: Diff 50583.Thu, Oct 31, 14:56
PiRK updated this revision to Diff 50584.Thu, Oct 31, 16:29

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

PiRK updated this revision to Diff 50585.Thu, Oct 31, 16:38

remove now useless functions and imports

Harbormaster completed remote builds in B30945: Diff 50584.Thu, Oct 31, 16:58
Harbormaster completed remote builds in B30946: Diff 50585.Thu, Oct 31, 17:27
tobias_ruck accepted this revision.Fri, Nov 1, 19: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

Merge those two imports in one big one

bitcoinsuite_core::{…}

This revision is now accepted and ready to land.Fri, Nov 1, 19:27
tobias_ruck added inline comments.Fri, Nov 1, 19:28
chronik/chronik-db/src/io/group_history.rs
223 ↗(On Diff #50575)

Ignore this—from an old diff

PiRK updated this revision to Diff 50621.Sat, Nov 2, 19:36

merge imports

Harbormaster completed remote builds in B30972: Diff 50621.Sat, Nov 2, 20:02
Closed by commit rABCbf00137a339a: [chronik] add scripthash option to the various script endpoints (authored by PiRK). · Explain WhySat, Nov 2, 22:43
This revision was automatically updated to reflect the committed changes.
PiRK added a commit: rABCbf00137a339a: [chronik] add scripthash option to the various script endpoints.
Fabien removed a child revision: D17037: [chronik] index scripts by scripthashes in the mempool.Tue, Nov 5, 13:26
PiRK mentioned this in rABC91a04f028842: [chronik] implement scripthash type for utxos endpoints.Mon, Nov 11, 07:35

Revision Contents
Changeset List

PathSize
chronik/
chronik-db/
src/
24 lines
io/
25 lines
chronik-http/
src/
64 lines
chronik-indexer/
src/
6 lines
query/
53 lines
test/
functional/
193 lines
test_framework/
4 lines

Diff 50585

View Options

chronik/chronik-db/src/group.rs

Loading...
View Options

chronik/chronik-db/src/io/group_history.rs

Loading...
View Options

chronik/chronik-http/src/handlers.rs

Loading...
View Options

chronik/chronik-indexer/src/indexer.rs

Loading...
View Options

chronik/chronik-indexer/src/query/group_history.rs

Loading...
View Options

test/functional/chronik_scripthash.py

Loading...
View Options

test/functional/test_framework/hash.py

Loading...