Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

get datums using pallas observer #1403

Merged
merged 69 commits into from
Feb 13, 2024
Merged

Conversation

falcucci
Copy link
Collaborator

@falcucci falcucci commented Dec 16, 2023

Content

Includes:

  • get current datums using the new pallas observer;
  • brings all compatibilities with the previous cardano-cli format;
  • uses the current cardano-cli as a default fallback for the remaining methods;

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

Comments

The automatic fallback mode has been implemented to make sure we progressively rollout the new integration.

Issue(s)

fixes #1315

@falcucci falcucci changed the title enhance mithril/cardano node communication to get datums using pallas observer get datums using pallas observer Dec 16, 2023
@falcucci falcucci changed the title get datums using pallas observer get datums using pallas observer [wip] Dec 16, 2023
mithril-common/Cargo.toml Outdated Show resolved Hide resolved
the specific changes include:
- removing the `println!` call in `mithril-common/src/chain_observer/cli_observer.rs`
- removing the `minicbor::encode` implementation for `metadatum` in `mithril-common/src/chain_observer/codec.rs`
- removing the `minicbor::encode` implementation for `constr` in `mithril-common/src/chain_observer/codec.rs`
- adding mock utxo data in `mithril-common/src/chain_observer/pallas_observer.rs`
removes the tag field from metadatum and its associated tests.
@falcucci falcucci marked this pull request as ready for review December 17, 2023 22:13
@falcucci falcucci changed the title get datums using pallas observer [wip] get datums using pallas observer Dec 18, 2023
@falcucci falcucci marked this pull request as draft December 19, 2023 14:24
- change the function signature of `inspect` to take a `vec<u8>` instead of `anycbor`
- remove the unnecessary decoding of `cbor` in the `inspect` function
- remove the unnecessary creation of `datum` as `anycbor` in the `inspect_datum` function
- remove the unnecessary decoding of `cbor` in the `inspect_datum` function
- change the comment description for the `inspect` function
@falcucci
Copy link
Collaborator Author

falcucci commented Jan 23, 2024

@jpraynaud I've just fixed a deserialization issue to consume the inline datum.

From now on I am not sure if we have an issue in the CI side. It needs further investigation but it worked on my machine as you can see in the pic bellow.

TLDR; the error we are getting now I could reproduce and it was related to fetch the epoch settings at

http://localhost:8080/aggregator/epoch-settings

At my machine I just restarted the tests and it worked again as expected but restarting the CI step doesnt have the same results.

CleanShot 2024-01-23 at 02 07 55@2x

@jpraynaud
Copy link
Member

jpraynaud commented Jan 23, 2024

@jpraynaud I've just fixed a deserialization issue to consume the inline datum.

From now on I am not sure if we have an issue in the CI side. It needs further investigation but it worked on my machine as you can see in the pic bellow.

TLDR; the error we are getting now I could reproduce and it was related to fetch the epoch settings at

http://localhost:8080/aggregator/epoch-settings

At my machine I just restarted the tests and it worked again as expected but restarting the CI step doesnt have the same results.

Hey @falcucci, I guess that you are probably running the previous version of the end to end test 🤔
Can you share the command that you are using to run the end to end test locally?

I have been able to reproduce the problem locally on my machine (it's the same problem as on the CI):

Error: Dependencies Builder can not create aggregator runner

Caused by:
    Dependency initialization error: «Error while building EraChecker» with additional nested error: 'Adapter Error message: «Reading from EraReader adapter raised an error: 'general error'.»
    
    Caused by:
        0: general error
        1: PallasChainObserver failed to get utxo
        2: failure decoding CBOR data
    
    Stack backtrace:
       0: anyhow::error::<impl core::convert::From<E> for anyhow::Error>::from
       1: mithril_aggregator::dependency_injection::builder::DependenciesBuilder::get_era_checker::{{closure}}
       2: mithril_aggregator::dependency_injection::builder::DependenciesBuilder::build_dependency_container::{{closure}}
       3: mithril_aggregator::commands::serve_command::ServeCommand::execute::{{closure}}
       4: mithril_aggregator::commands::MainCommand::execute::{{closure}}
       5: tokio::runtime::park::CachedParkThread::block_on
       6: mithril_aggregator::main
       7: std::sys_common::backtrace::__rust_begin_short_backtrace
       8: std::rt::lang_start::{{closure}}
       9: core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once
                 at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/ops/function.rs:284:13
      10: std::panicking::try::do_call
                 at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/panicking.rs:552:40
      11: std::panicking::try
                 at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/panicking.rs:516:19
      12: std::panic::catch_unwind
                 at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/panic.rs:142:14
      13: std::rt::lang_start_internal::{{closure}}
                 at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/rt.rs:148:48
      14: std::panicking::try::do_call
                 at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/panicking.rs:552:40
      15: std::panicking::try
                 at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/panicking.rs:516:19
      16: std::panic::catch_unwind
                 at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/panic.rs:142:14
      17: std::rt::lang_start_internal
                 at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/rt.rs:148:20
      18: std::rt::lang_start
      19: <unknown>
      20: __libc_start_main
      21: _start'.

It looks like the queries_v16::get_utxo_by_address(statequery, era, addrs) function is returning the CBOR data decoding error.

@falcucci
Copy link
Collaborator Author

falcucci commented Jan 23, 2024

@jpraynaud everything here is updated til yesterday, this error is the fix that I've mentioned that I already made and it's working here, but cant reproduce anymore.

is there any change that the aggregator isnt being updated with the bugfix in the CI?

the tests are running successfully here using:

./mithril-end-to-end -vvv --bin-directory ../../target/release --work-directory=./artifacts --devnet-scripts-directory=./mithril-test-lab/mithril-devnet --skip-cardano-bin-download --mithril-era=thales  --cardano-hard-fork-latest-era-at-epoch 1000

- remove the `use anyhow::context;` from the start of the file.
- add `use anyhow::context;` within the `cfg_fs!` block.
@falcucci
Copy link
Collaborator Author

@jpraynaud just to make it documented here: got it running changing the node version to 8.1.2

the local state queries changed between node versions, so using 8.7.3 breaks it since pallas has not support for now.

@falcucci
Copy link
Collaborator Author

@jpraynaud FYI this has been addressed at txpipe/pallas#385 and a proposed fix at txpipe/pallas#386

- change source of `pallas-addresses`, `pallas-codec`, `pallas-network`, `pallas-primitives`, `pallas-traverse`, and `pallas-crypto` in `cargo.lock` and `mithril-common/cargo.toml` from crates.io to a specific branch `fix/legacy-transaction-utxo-by-address` on repository `https://github.com/falcucci/pallas.git`
- replace `values` type with `postalonsotransactionoutput` as an argument in `pallaschainobserver` methods including `get_datum_tag`, `inspect_datum`, and `serialize_datum` in `mithril-common/src/chain_observer/pallas_observer.rs`
- update filter logic in `serialize_datum` to match `transactionoutput::map`
- update test data to match the new `transactionoutput` type changes
- update `inventory` from version `0.3.14` to `0.3.15` with new checksum.
- update multiple `pallas` packages (addresses, codec, crypto, network, primitives, traverse) from version `0.21.0` to `0.22.0` with updated git source reference.
- update `snow` from version `0.9.5` to `0.9.6` with new checksum.
- in `pallas_observer.rs`, replace `transactionoutput::map` with `transactionoutput::current` for utxo iteration and test values instantiation.
@jpraynaud
Copy link
Member

Hi @falcucci, now it's working with 8.7.3 but it seems that there is a backward compatibility problem with 8.1.2.

Here is the job that fails in the CI: https://github.com/input-output-hk/mithril/actions/runs/7668627456/job/20902207802

- modify the pallaschainobserver implementation to serialize the output datum inline if present, instead of unconditionally.
@falcucci
Copy link
Collaborator Author

falcucci commented Jan 26, 2024

@jpraynaud it wasn't upstream updated, fixed :)

when we have a new pallas version i'll bump it and notify you

@jpraynaud
Copy link
Member

@jpraynaud it wasn't upstream updated, fixed :)

when we have a new pallas version i'll bump it and notify you

Great news @falcucci! Once the new pallas version is available, we'll merge the PR 🎉

Thanks for the good work!

mithril-common/src/lib.rs Fixed Show fixed Hide fixed
mithril-common/src/lib.rs Fixed Show fixed Hide fixed
- updated `pallas-addresses`, `pallas-codec`, `pallas-network`, `pallas-primitives`, `pallas-traverse`, and `pallas-crypto` dependencies from git to version `0.23.0`.
- removed the `cfg_fs` macro from `lib.rs` in the `mithril-common` module.
@falcucci
Copy link
Collaborator Author

falcucci commented Feb 12, 2024

@jpraynaud just updated from upstream and bumped the new pallas version.

@jpraynaud
Copy link
Member

Hi @falcucci, can you rebase once more on the main branch and bump the patch version of mithril-common?
I'll merge the PR thereafter 🚀

- bump up version of `mithril-common` from `0.3.0` to `0.3.1`.
@falcucci
Copy link
Collaborator Author

@jpraynaud done :)

@jpraynaud jpraynaud merged commit 3a1992f into input-output-hk:main Feb 13, 2024
36 of 37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance Mithril/Cardano node communication
6 participants