enhance mithril/cardano node communication #1348
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
the `cardano-cli-chain-observer` is the default for now
…-network` packages in the `cargo.lock` file from `git+https://github.com/txpipe/pallas.git#2b5f22def2a76341337417533b7116e13ac9880c` to `git+https://github.com/txpipe/pallas.git?branch=main#c772da69ec04d04df7d59da878f486c04ec16f11`. this allows us to access the most up-to-date version of the packages and ensure that we are always using the most recent version. ```diff diff --git a/cargo.lock b/cargo.lock index d8e09d5b9dea..cc1ac0e50355 100644 --- a/cargo.lock +++ b/cargo.lock @@ -2737,7 +2737,7 @@ dependencies = [ [[package]] name = "pallas-codec" version = "0.19.1" -source = "git+https://github.com/txpipe/pallas.git#2b5f22def2a76341337417533b7116e13ac9880c" +source = "git+https://github.com/txpipe/pallas.git?branch=main#c772da69ec04d04df7d59da878f486c04ec16f11" dependencies = [ "hex", "minicbor", @@ -2748,7 +2748,7 @@ dependencies = [ [[package]] name = "pallas-crypto" version = "0.19.1" -source = "git+https://github.com/txpipe/pallas.git#2b5f22def2a76341337417533b7116e13ac9880c" +source = "git+https://github.com/txpipe/pallas.git?branch=main#c772da69ec04d04df7d59da878f486c04ec16f11" dependencies = [ "cryptoxide", "hex", @@ -2761,7 +2761,7 @@ dependencies = [ [[package]] name = "pallas-network" version = "0.19.1" -source = "git+https://github.com/txpipe/pallas.git#2b5f22def2a76341337417533b7116e13ac9880c" +source = "git+https://github.com/txpipe/pallas.git?branch=main#c772da69ec04d04df7d59da878f486c04ec16f11" dependencies = [ "byteorder", "hex", ```
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
cargo-doc found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
There was a problem hiding this comment.
clippy found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
falcucci
changed the title
```diff
diff --git a/mithril-common/src/chain_observer/pallas_observer.rs b/mithril-common/src/chain_observer/pallas_observer.rs
index b5a2c271e7db..43b3c1308aa1 100644
--- a/mithril-common/src/chain_observer/pallas_observer.rs
+++ b/mithril-common/src/chain_observer/pallas_observer.rs
@@ -26,7 +26,7 @@ impl pallaschainobserver {
let fallback = cardanoclichainobserver::new(box::new(super::cardanoclirunner::new(
cli_path.to_owned(),
socket.to_owned(),
- network.clone(),
+ network,
)));
self {
```
…allaschainobserver` impl in `mithril-common/src/chain_observer/pallas_observer.rs` to acquire and release the state query during the chain sync process. this ensures that the state query is properly handled and the system runs as expected.
```diff
diff --git a/mithril-common/src/chain_observer/pallas_observer.rs b/mithril-common/src/chain_observer/pallas_observer.rs
index 43b3c1308aa1..6d9affa7f873 100644
--- a/mithril-common/src/chain_observer/pallas_observer.rs
+++ b/mithril-common/src/chain_observer/pallas_observer.rs
@@ -69,6 +69,8 @@ impl chainobserver for pallaschainobserver {
let statequery = client.statequery();
+ statequery.acquire(none).await.unwrap();
+
// todo: maybe implicitely get the current era as default
let era = queries_v16::get_current_era(statequery)
.await
@@ -78,6 +80,9 @@ impl chainobserver for pallaschainobserver {
.await
.map_err(|err| chainobservererror::general(err.into()))?;
+ statequery.send_release().await.unwrap();
+ statequery.send_done().await.unwrap();
+
client.chainsync().send_done().await.unwrap();
client.abort();
```
this commit ensures that the state query is properly handled and the system runs as expected.
removes the `chain_observer_type` field from the `defaultconfiguration` struct defined in `configuration.rs`. this field was used to specify the type of chain observer adapter, but was unused and therefore can be safely removed.
the removed lines are as follows:
```
- /// type of chain observer adapter.
- pub chain_observer_type: string,
```
and the resulting code is:
```diff
diff --git a/mithril-aggregator/src/configuration.rs b/mithril-aggregator/src/configuration.rs
index 97e9315c4a51..e233079cc759 100644
--- a/mithril-aggregator/src/configuration.rs
+++ b/mithril-aggregator/src/configuration.rs
@@ -273,9 +273,6 @@ pub struct defaultconfiguration {
/// signer importer run interval default setting
pub signer_importer_run_interval: u64,
-
- /// type of chain observer adapter.
- pub chain_observer_type: string,
}
impl default for defaultconfiguration {
@@ -289,7 +286,6 @@ impl default for defaultconfiguration {
snapshot_store_type: "local".to_string(),
snapshot_uploader_type: "gcp".to_string(),
era_reader_adapter_type: "bootstrap".to_string(),
- chain_observer_type: "cardano-cli-chain-observer".to_string(),
reset_digests_cache: "false".to_string(),
disable_digests_cache: "false".to_string(),
snapshot_compression_algorithm: "zstandard".to_string(),
```
this commit removes the unnecessary field to simplify the configuration struct and avoid any potential confusion.
falcucci
force-pushed
the
feat/pallas-observer
branch
2 times, most recently
from
8a84226
to
ab7eec6
Compare
falcucci
force-pushed
the
feat/pallas-observer
branch
from
f378ba2
to
788e716
Compare
scarmuega
reviewed
Nov 17, 2023
scarmuega
reviewed
Nov 17, 2023
- removed the `epochno` type from the `localstate::queries_v16` module - updated test code to use the new array format for the `epochno` type - updated the `get_current_epoch_with_fallback` test to use the new array format for the `epochno` type - updated the `get_epoch_range` test to use the new array format for the `epochno` type - updated the `observer.get_current_epoch()` method to use the new array format for the `epochno` type
jpraynaud
requested review from
ghubertpalo
and removed request for
scarmuega
jpraynaud
requested changes
Nov 24, 2023
|
@falcucci Your commits must be signed in order to be merged in the main branch, could you re-push them? |
|
@falcucci When I run Are there any prerequisites before running the tests? Is it possible that it's something to do with my local environment? |
@dlachaumepalo done!
@dlachaumepalo there is no changes to setup the tests, could you share the error trace and the command you use to run it? is it getting the correct that's werid cuz in the CI which is an isolated env works as expected |
- added `pallaschainobserver` to `chain_observer` namespace - added `pallaschainobserver::new_with_fallback` to create the observer with a fallback - added a match for the `executionenvironment` in the `build_chain_observer` method to use the `pallaschainobserver` if in production mode.
In fact, it's fine. The problem was the same as the one you reported. Had to specify a shorter path for the parameter Thanks for your response Alex! |
- move `test_cli_runner` module from `test_utils` to `chain_observer` - change `testclirunner`'s visibility from `pub` to `pub(crate)` - update usages of `testclirunner` in `chain_observer::cli_observer` and `chain_observer::pallas_observer` to refer to the new module path
- refactored of the test setup code into a separate `setup_server` function
- removed unnecessary `println!` call to improve readability
- added documentation to the `setup_server` function
```diff
diff --git a/mithril-common/src/chain_observer/pallas_observer.rs b/mithril-common/src/chain_observer/pallas_observer.rs
index fe7bc2f3c626..225b8398af5c 100644
--- a/mithril-common/src/chain_observer/pallas_observer.rs
+++ b/mithril-common/src/chain_observer/pallas_observer.rs
@@ -199,14 +199,13 @@ mod tests {
temp_dir
}
- #[tokio::test]
- async fn get_current_epoch_with_fallback() {
- let server = tokio::spawn({
+ /// sets up a mock server.
+ async fn setup_server() -> tokio::task::joinhandle<()> {
+ tokio::spawn({
async move {
let temp_dir = create_temp_dir("pallas_chain_observer_test");
let socket_path = temp_dir.join("node.socket").as_path().to_owned();
if socket_path.exists() {
- println!("removing previous socket");
fs::remove_file(&socket_path).expect("previous socket removal failed");
}
@@ -224,8 +223,12 @@ mod tests {
let result = mock_server(&mut server).await;
server.statequery().send_result(result).await.unwrap();
}
- });
+ })
+ }
+ #[tokio::test]
+ async fn get_current_epoch_with_fallback() {
+ let server = setup_server().await;
let client = tokio::spawn(async move {
let socket_path = std::env::temp_dir().join("pallas_chain_observer_test/node.socket");
let fallback = cardanoclichainobserver::new(box::<testclirunner>::default());
@@ -234,10 +237,11 @@ mod tests {
cardanonetwork::testnet(10),
fallback,
);
- let epoch = observer.get_current_epoch().await.unwrap().unwrap();
- assert_eq!(epoch, 8);
+ observer.get_current_epoch().await.unwrap().unwrap()
});
- _ = tokio::join!(client, server);
+ let (_, client_res) = tokio::join!(server, client);
+ let epoch = client_res.expect("client failed");
+ assert_eq!(epoch, 8);
}
}
```
|
@jpraynaud all suggestions has been applied, the only review left for discussion is #1348 (comment) thank you :) |
|
bumped crates versions accordingly. |
falcucci
force-pushed
the
feat/pallas-observer
branch
from
b14005d
to
8bb39c9
Compare
the changes made in this commit are: - the return type of `get_fallback()` has been changed from `stdresult<&cardanoclichainobserver>` to `&cardanoclichainobserver` - the call to `map_err()` has been removed from the usage of `get_fallback()` within `get_current_datums()`, `get_current_stake_distribution()`, and `get_current_kes_period()` - the return type of `get_current_datums()` has been changed from `stdresult<vec<txdatum>, chainobservererror>` to `result<vec<txdatum>, chainobservererror>`
falcucci
force-pushed
the
feat/pallas-observer
branch
from
87f9b23
to
323ecdc
Compare
dlachaume
approved these changes
Nov 29, 2023
|
@scarmuega @falcucci the merged PR has been deployed on the It looks like it's working smoothly so far 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Content
Includes:
Pre-submit checklist
Comments
The automatic fallback mode has been implemented to make sure we progressively rollout the new integration.
Issue(s)
fixes #1315