⚙ D17037 [chronik] index scripts by scripthashes in the mempool

PiRK created this revision.Tue, Oct 29, 14:08

Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, Oct 29, 14:08

PiRK requested review of this revision.Tue, Oct 29, 14:08

Harbormaster completed remote builds in B30902: Diff 50526.Tue, Oct 29, 14:13

The build failed due to an unexpected infrastructure outage. The administrators have been notified to investigate. Sorry for the inconvenience.

rebase

Harbormaster completed remote builds in B30904: Diff 50529.Tue, Oct 29, 15:52

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.31s

   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:

    Checking serde_urlencoded v0.7.1
    Checking futures-executor v0.3.30
    Checking serde_path_to_error v0.1.16
    Checking hyper-util v0.1.3
   Compiling chronik-proto v0.1.0 (/work/chronik/chronik-proto)
    Checking base64 v0.21.7
    Checking matchit v0.7.3
    Checking sync_wrapper v1.0.1
    Checking unicode-segmentation v1.11.0
    Checking unindent v0.2.3
    Checking tower-http v0.5.2
    Checking futures v0.3.30
    Checking convert_case v0.6.0
    Checking toml_edit v0.22.14
   Compiling chronik-bridge v0.1.0 (/work/chronik/chronik-bridge)
   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)
error: unnecessary closure used to substitute value for `Option::None`
   --> chronik/chronik-db/src/io/group_history.rs:225:30
    |
225 |           let cf_member_hash = self
    |  ______________________________^
226 | |             .cf_member_hash
227 | |             .ok_or_else(|| GroupDoesNotImplementMemberHash())?;
    | |______________-----------------------------------------------^
    |                |
    |                help: use `ok_or(..)` instead: `ok_or(GroupDoesNotImplementMemberHash())`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_lazy_evaluations
    = note: `-D clippy::unnecessary-lazy-evaluations` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::unnecessary_lazy_evaluations)]`

error: redundant closure
   --> chronik/chronik-db/src/io/group_history.rs:227:25
    |
227 |             .ok_or_else(|| GroupDoesNotImplementMemberHash())?;
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `GroupDoesNotImplementMemberHash`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure
    = note: `-D clippy::redundant-closure` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::redundant_closure)]`

error: this boolean expression can be simplified
  --> chronik/chronik-db/src/mem/group_history.rs:35:36
   |
35 |         let member_hash_index = if !conf.cf_member_hash_name.is_none() {
   |                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `conf.cf_member_hash_name.is_some()`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#nonminimal_bool
   = note: `-D clippy::nonminimal-bool` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(clippy::nonminimal_bool)]`

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

PiRK added a child revision: D17043: [chronik] implement scripthash type for utxos endpoints.Wed, Oct 30, 09:25

simplify expressions

Harbormaster completed remote builds in B30916: Diff 50549.Wed, Oct 30, 11:18

tobias_ruck requested changes to this revision.Wed, Oct 30, 17:16

tobias_ruck added a subscriber: tobias_ruck.

tobias_ruck added inline comments.

chronik/chronik-db/src/mem/group_history.rs
35–39 ↗	(On Diff #50549)	`let member_hash_index = conf.cf_member_hash_name.is_some().then(HashMap::new);` might also work, not sure
110 ↗	(On Diff #50549)	we can point directly into the mempool and let the caller decide whether to allocate or not
113 ↗	(On Diff #50549)	no need to allocate here
114 ↗	(On Diff #50549)	I think this should work?
test/functional/chronik_scripthash.py
167 ↗	(On Diff #50549)	You should use the `check_num_txs` also for mempool txs, or change it upstream to not care about mempool txs Also it might be good to test txs being both in the mempool and in blocks for the same script, and handling that gracefully (esp. in `history`)

This revision now requires changes to proceed.Wed, Oct 30, 17:16

PiRK added inline comments.Thu, Oct 31, 13:22

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

110 ↗

(On Diff #50549)

I wasn't able to make this work.

If I just add .as_deref():

error[E0515]: cannot return value referencing temporary value
   --> chronik/chronik-db/src/mem/group_history.rs:108:20
    |
108 |                return member_hash_index
    |   ____________________
    |  |
109 | ||                 .get(member_hash.to_be_bytes().as_ref())
110 | ||                 .cloned().as_deref();
    | ||_________________________-__________^
    |  |_________________________|
    |                            temporary value created here

If I removed .cloned() before adding as_deref()

error[E0308]: mismatched types
   --> chronik/chronik-db/src/mem/group_history.rs:108:20
    |
106 |       ) -> Option<&[u8]> {
    |            ------------- expected `std::option::Option<&[u8]>` because of return type
107 |           if let Some(member_hash_index) = &self.member_hash_index {
108 |               return member_hash_index
    |
109 | |                 .get(member_hash.to_be_bytes().as_ref())
110 | |                 .as_deref();
    | |___________________________^
    |
    = note: expected enum `std::option::Option<&[u8]>`
               found enum `std::option::Option<&Vec<u8>>`
help: try using `.map(|v| &**v)` to convert `std::option::Option<&Vec<u8>>` to `std::option::Option<&[u8]>`
    |
110 |                 .as_deref().map(|v| &**v);
    |                            ++++++++++++++

test/functional/chronik_scripthash.py

167 ↗

(On Diff #50549)

I'm not sure what you mean. We currently do that already, don't we?

We only check a single script (OP_TRUE) and we test both scenarios, transaction only in blocks (confirmed_txs) or txs both in block and mempool (via unconfirmed_txs)

For history we check

assert_equal(script_history.num_txs, num_block_txs + num_mempool_txs)

then_some, don't allocate

Harbormaster completed remote builds in B30943: Diff 50582.Thu, Oct 31, 14:34

rebase

Harbormaster completed remote builds in B30950: Diff 50589.Thu, Oct 31, 20:36

[chronik] index scripts by scripthashes in the mempool
Needs ReviewPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 50582

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

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

test/functional/chronik_scripthash.py

[chronik] index scripts by scripthashes in the mempoolNeeds ReviewPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 50582

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

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

test/functional/chronik_scripthash.py

[chronik] index scripts by scripthashes in the mempool
Needs ReviewPublic
Actions

Revision Contents
Changeset List