Explorer crate #3350

ecioppettini · 2021-06-03T04:15:09Z

see #3227

~~the reason the CI fails should be solved by #3595~~

~~also, this is (re?)based on top of #3662 , that one should be merged first.~~

this PR:

removes the explorer module from jormungandr
moves the implementation to a new crate in the workspace
changes the explorer_sanity_test in the integration tests to launch an explorer and test against it so it keeps working.
adds a simple tally implementation that doesn't use the ledger (because there is no ledger here). This is mostly to keep the api working without replacing it with an unimplemented error, but it may need a rework later.

zeegomo

First look, looks good overall

explorer/src/api/graphql/mod.rs

explorer/src/api/graphql/scalars.rs

explorer/src/api/mod.rs

zeegomo · 2021-12-07T10:37:20Z

explorer/src/db/mod.rs

@@ -801,48 +677,22 @@ fn apply_block_to_vote_plans(
                    }
                }
                Certificate::VoteTally(vote_tally) => {
-                    use chain_impl_mockchain::vote::PayloadType;
+                    let decrypted_tally = vote_tally.tally_decrypted().unwrap();


What about public votes?

I think this should be addressed now

explorer/src/db/tally.rs

explorer/src/logging.rs

zeegomo · 2021-12-07T10:51:15Z

explorer/src/indexer.rs

+        let successful = self.db.set_tip(tip).await;
+
+        if !successful {
+            let mut guard = self.tip_candidate.lock().await;


I'm missing a bit of context for this two level tip update

Well, it's kinda hacky I guess.

We don't have any guarantees about the order of events, and if we set a block as the tip, but haven't indexed the block yet, queries in between will fail.

In the past, inside the node, we broadcasted tip updates before broadcasting the blocks. I'm not sure if that changed actually, because I believe you refactored that, but I wanted the explorer to keep working regardless of the order.

And here, because the tip and block subscriptions are different streams, I think it may be possible for this to happen regardless of the order they are emitted in jormungandr.

So db.set_tip will fail if the block was not indexed yet? I would add a comment explaining this behavior in the code though, as it is not immediate.

PS: I don't remember now if there's a particular order in that, but I would guess tip updates follow block broadcasts.

Well, I changed the return type to a Result<(), BlockNotFound> instead, as it is a bit more self-documenting.

zeegomo

I think there are still some explorer-related files left in the node

explorer/Cargo.toml

explorer/src/api/graphql/certificates.rs

explorer/src/db/mod.rs

explorer/src/db/persistent_sequence.rs

testing/jormungandr-integration-tests/src/jormungandr/mempool.rs

zeegomo

I think this is now in a shape that can go in.
There are parts that could be further polished and/or improved in the implementation, but I'll leave this for future PRs, as it's mostly code we already had.

Good work!

eugene-babichenko

Looks good in general, but definitely needs a rebase. I was also thinking that some parts (I spotted at least logging, REST API setup and stop signal handler) are basically the same as in jormungandr.

We should probably have a reusable module with these.

eugene-babichenko · 2022-02-07T12:54:17Z

explorer/Cargo.toml

+serde_yaml = "0.8.13"
+structopt = "0.3.15"
+thiserror = "1.0.20"
+anyhow = "1.0.41"


Is it used? I would not use anyhow as we are trying to use thiserror everywhere for consistency.

Both are used, since I found it quite convenient for the top level (user) errors, in which I only wanted an error message for the user, but it's not strictly necessary.

like, with these things:

let mut client = Client::connect(settings.node.clone()) .await .context("Couldn't establish connection with node") .map_err(Error::UnrecoverableError)?; let sync_stream = client .sync_multiverse(vec![]) .await .context("Failed to establish bootstrap stream") .map_err(Error::UnrecoverableError)?; let block_events = client .block_subscription() .await .context("Failed to establish block subscription") .map_err(Error::UnrecoverableError)?; let tip_events = client .tip_subscription() .await .context("Failed to establish tip subscription") .map_err(Error::UnrecoverableError)?;

Since they are in the main function, having enum variants for each case is not really that important (because no one will pattern match on them), and IMO being able to just provide some specific info directly in there makes it easier to provide good error messages since otherwise errors tend to get more generic.

That said, it's certainly possible to achieve the same with just thiserror, and it's not used extensively, but I don't think they are really alternatives, I feel it's more like a distinction between library errors vs application errors.

testing/jormungandr-testing-utils/src/testing/node/explorer/wrappers.rs

eugene-babichenko · 2022-02-16T20:13:58Z

Gah, there is still one conflicting file.

eugene-babichenko · 2022-02-16T20:15:15Z

explorer/src/api/graphql/config_param.rs

@@ -528,7 +511,7 @@ impl From<&ConfigParamLib> for ConfigParam {
                Self::TransactionMaxExpiryEpochs(v.into())
            }
            #[cfg(feature = "evm")]
-            ConfigParamLib::EvmParams(v) => Self::EvmParams(v.into()),
+            ConfigParamLib::EvmParams(_) => unimplemented!("evm support"),


Are we just going to drop EVM support?

well, not drop it, since but we don't really have support for this in jormungandr either:

jormungandr/jormungandr/src/explorer/graphql/config_param.rs

Line 447 in 97f3439

fn from(_: &chain_impl_mockchain::config::EvmConfig) -> Self {

But I will just put the same thing here for consistency and to avoid the panic.

eugene-babichenko · 2022-02-16T20:17:48Z

And don't forget to add the changelog entry :)

this makes the code a bit more self-documenting than with a boolean, even if the error value is not actually used here.

because otherwise the function will panic for public tally, as there is no decrypted tally

it was returning the opposite value

to account for slow IO, or performance issues when running the test

this allows using it in other tests, like in `retire_stake_pool_explorer` and other integration tests

by just panicking, proper suppport can be done later

ecioppettini self-assigned this Jun 3, 2021

ecioppettini force-pushed the explorer-crate branch from b95cab0 to 68764c7 Compare June 4, 2021 00:01

ecioppettini force-pushed the notifier-grpc branch from e0b006a to 44efd96 Compare June 8, 2021 14:36

ecioppettini force-pushed the explorer-crate branch from bcc4e1c to 50dae54 Compare June 8, 2021 16:39

ecioppettini mentioned this pull request Jun 25, 2021

Add log lib #3395

Closed

ecioppettini force-pushed the notifier-grpc branch from 3b8f772 to 631ca8a Compare September 11, 2021 03:19

ecioppettini force-pushed the explorer-crate branch 2 times, most recently from e3c580b to 54297c5 Compare September 17, 2021 20:21

This was referenced Sep 17, 2021

Explorer crate sanakirja db #3605

Closed

add explorer library input-output-hk/chain-libs#651

Closed

ecioppettini force-pushed the explorer-crate branch from 54297c5 to f6d9f2c Compare October 6, 2021 18:45

ecioppettini force-pushed the explorer-crate branch 2 times, most recently from 2ae0fb2 to 78c28b0 Compare October 20, 2021 02:13

ecioppettini changed the base branch from notifier-grpc to master October 20, 2021 02:15

ecioppettini force-pushed the explorer-crate branch from 78c28b0 to 1a0d59a Compare November 10, 2021 23:31

ecioppettini force-pushed the explorer-crate branch from 290a9c6 to 1160cef Compare November 18, 2021 18:59

ecioppettini force-pushed the explorer-crate branch from 1160cef to 46ed8e1 Compare November 25, 2021 21:51

ecioppettini marked this pull request as ready for review November 26, 2021 14:36

ecioppettini force-pushed the explorer-crate branch from 5d33f2f to 89f0a43 Compare November 29, 2021 04:43

zeegomo reviewed Dec 7, 2021

View reviewed changes

ecioppettini force-pushed the explorer-crate branch from 926a881 to 1a72ae0 Compare December 7, 2021 16:26

eugene-babichenko added the A-explorer Area: Explorer API and backend label Dec 8, 2021

zeegomo suggested changes Dec 14, 2021

View reviewed changes

ecioppettini force-pushed the explorer-crate branch from 217e2e7 to 432c571 Compare December 23, 2021 13:50

zeegomo approved these changes Dec 24, 2021

View reviewed changes

eugene-babichenko reviewed Feb 8, 2022

View reviewed changes

ecioppettini force-pushed the explorer-crate branch from b5dc963 to 81e2b8d Compare February 14, 2022 20:06

eugene-babichenko reviewed Feb 16, 2022

View reviewed changes

ecioppettini and others added 27 commits March 7, 2022 14:40

fix tests: use rest for current time instead of explorer

90696bd

return a Result in set_tip

226801b

this makes the code a bit more self-documenting than with a boolean, even if the error value is not actually used here.

remove unused Result alias

ff0e32e

build explorer in actions test pipeline too

139d2b3

handly tally type at an earlier point

c079a66

because otherwise the function will panic for public tally, as there is no decrypted tally

log listen address when cors is not enabled

3888401

collect tally result instead of looping

395626c

remove line added by mistake

0848d11

fix PersistentSequence::is_empty

648cb15

it was returning the opposite value

remove unused files

88f5d53

impl From instead of TryFrom

13df3cf

remove outdated comment

a16428d

use Certificate::from instead of try_from

a1b1abd

fix tests: explorer is not a node setting anymore

27886b2

add retry when querying the explorer

c961951

to account for slow IO, or performance issues when running the test

move explorer process spawn to JormungandrProcess

cc2f2e1

this allows using it in other tests, like in `retire_stake_pool_explorer` and other integration tests

remove unnecessary thread::sleep in test

3385e50

handle evm config param

aceae5b

by just panicking, proper suppport can be done later

fix compilation issues

bf383b3

port EvmConfig code from jormungandr

a0d7860

add changelog entry

11223cb

update Cargo.lock

2312148

explorer: remove EncryptedVoteTally

7891df5

port/update config_param.rs

008c840

refactor subscriber init

fd6d299

test: send explorer logs to stdout

a5b1774

explorer sanity test: reduce attemps to 20

70c6000

ecioppettini force-pushed the explorer-crate branch from 43f0f58 to 70c6000 Compare March 7, 2022 21:20

ecioppettini merged commit acf37fe into master Mar 9, 2022

ecioppettini deleted the explorer-crate branch March 9, 2022 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explorer crate #3350

Explorer crate #3350

ecioppettini commented Jun 3, 2021 •

edited

Loading

zeegomo left a comment

zeegomo Dec 7, 2021

ecioppettini Dec 13, 2021

zeegomo Dec 7, 2021

ecioppettini Dec 7, 2021

zeegomo Dec 7, 2021

ecioppettini Dec 7, 2021

zeegomo left a comment

zeegomo left a comment

eugene-babichenko left a comment

eugene-babichenko Feb 7, 2022

ecioppettini Feb 14, 2022

eugene-babichenko commented Feb 16, 2022

eugene-babichenko Feb 16, 2022

ecioppettini Feb 16, 2022

eugene-babichenko commented Feb 16, 2022

Explorer crate #3350

Explorer crate #3350

Conversation

ecioppettini commented Jun 3, 2021 • edited Loading

zeegomo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeegomo left a comment

Choose a reason for hiding this comment

zeegomo left a comment

Choose a reason for hiding this comment

eugene-babichenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugene-babichenko commented Feb 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugene-babichenko commented Feb 16, 2022

ecioppettini commented Jun 3, 2021 •

edited

Loading