Conversation
Deploying blockfrost-platform with
|
| Latest commit: |
704ddab
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3e28e244.blockfrost-platform.pages.dev |
| Branch Preview URL: | https://feat-tx-evaluation-rebased.blockfrost-platform.pages.dev |
michalrus
left a comment
There was a problem hiding this comment.
Hey, I started reviewing, but then noticed that it's a draft! Left a few comments either way :)
c702f03 to
a53a53c
Compare
3bcc5c5 to
e67b1a9
Compare
…ra tx-gen features)
6980be7 to
f269525
Compare
|
Maybe let's have a simpler merge commit for the current conflicts? 🥺 Unless you're using |
michalrus
left a comment
There was a problem hiding this comment.
One quick comment about the other repo (Pallas), and now I'll process the rest (here):
| pallas-addresses = { git = "https://github.com/blockfrost/pallas.git", tag = "blockfrost-platform-0.0.3-alpha6" } | ||
| pallas-codec = { git = "https://github.com/blockfrost/pallas.git", tag = "blockfrost-platform-0.0.3-alpha6" } | ||
| pallas-crypto = { git = "https://github.com/blockfrost/pallas.git", tag = "blockfrost-platform-0.0.3-alpha6" } | ||
| pallas-hardano = { git = "https://github.com/blockfrost/pallas.git", tag = "blockfrost-platform-0.0.3-alpha6" } |
There was a problem hiding this comment.
BTW, this tag resolves to:
Which is a part of:
I have linked this PR in description of this PR.
unwraps on calculations done on user-supplied data, so similar issues that were tackled in this PR.
Could you fix these issues there, so that they accept? And I'll review that one, too. Because otherwise, we still have unwraps in our code, just hidden under these dependency changes that the upstream didn’t approve.
There was a problem hiding this comment.
Thanks for flagging this. I have an open tab for the fixes and I am already working on it.
Background:
I opened that PR long before the AI reviews came in (major part of the PR), it was waiting for a merge for a while. So I doubt that that PR will get merged even after I satisfy the CodeRabbit.
CodeRabbit findings are valid. But the AI flagged topics are already present throughout the existing codebase. My code followed the existing conventions, but I'm happy to raise the bar on the new code I'm adding. Meanwhile, this should not be a blocker on this PR.
There was a problem hiding this comment.
Hmmm, but what do you think about this comment txpipe/pallas#624 (comment)? I'm not sure if we should have unwraps in production code, but I haven’t reviewed that Pallas PR myself yet, just looked at their AI review.
There was a problem hiding this comment.
I mean we got rid of unwraps here, so why not there? It's also the code we're running just in a different repository.
There was a problem hiding this comment.
Even though we wrote it, these are in a completely different repository. I am not saying we should not fix problems there, I am saying it should not be a blocker to merge this PR. Because now we've started reviewing another PR right now.
There was a problem hiding this comment.
And they (Pallas) haven’t done it for us, because that PR is not merged yet
There was a problem hiding this comment.
(So I'll just make that proper review there, too, maybe their bot is lying 🤞 )
There was a problem hiding this comment.
By the way, the bot's points are valid. I am fixing them.
There was a problem hiding this comment.
Okay – I've just submitted my review there, a couple of comments:
michalrus
left a comment
There was a problem hiding this comment.
Okay – another round done! The most important comment is about blockfrost-tests, I think.
crates/gateway/Cargo.toml
Outdated
| [dev-dependencies] | ||
| reqwest.workspace = true | ||
| rstest.workspace = true | ||
| tokio-tungstenite.workspace = true |
There was a problem hiding this comment.
Surprising – why would this PR need to change this? Hmmm. If it's failing checks, then why is the main check passing? 🤔
There was a problem hiding this comment.
Yeah, I didn't notice this difference between CI and my PR. Tho:
> nix develop --command cargo shear
warning: Git tree '/home/sefa/dev/blockfrost-platform' is dirty
shear/misplaced_dependency
× misplaced dependency `reqwest`
╭─[crates/gateway/Cargo.toml:25:1]
24 │ chrono = { workspace = true, features = ["serde"] }
25 │ reqwest.workspace = true
· ───┬───
· ╰── only used in dev targets
26 │ blockfrost.workspace = true
╰────
help: move this dependency to `[dev-dependencies]`
shear/misplaced_dependency
× misplaced dependency `tokio-tungstenite`
╭─[crates/gateway/Cargo.toml:14:1]
13 │ futures-util.workspace = true
14 │ tokio-tungstenite.workspace = true
· ────────┬────────
· ╰── only used in dev targets
15 │ tungstenite.workspace = true
╰────
help: move this dependency to `[dev-dependencies]`
| if version != 6 { | ||
| Err(BlockfrostError::conflicting_ogmios_version()) | ||
| } else { | ||
| let tx_cbor = binary_or_hex_heuristic(request.transaction.cbor.as_bytes()); |
There was a problem hiding this comment.
Hmmm, I wonder, won’t this always be hex-encoded? tx_request, and therefore the request variant will always be some kind of JSON. You think someone would pass:
{
"cbor": "\u0001\u0002\u0003"
}etc.? Is this format used?
The same question for other instances below.
There was a problem hiding this comment.
@michalrus Yes true, not Ogmios related. Removed in 69f459d.
| }; | ||
| use reqwest::Method; | ||
|
|
||
| /// |
There was a problem hiding this comment.
How does this look in cargo doc? Do they treat first non-empty comment line as the first comment line? Or is the first line empty? Is it a doc comment /// or a regular one, //?
There was a problem hiding this comment.
This will produce an empty line. Removed in f71720d.
| @@ -37,5 +37,7 @@ | |||
| "/epochs/latest/parameters", | |||
| "/genesis", | |||
| "/addresses/{address}/transactions", | |||
| "/addresses/{address}/utxos" | |||
| "/addresses/{address}/utxos", | |||
| "/utils/txs/evaluate", | |||
There was a problem hiding this comment.
A-ha! 🔍
You can see that blockfrost-tests are failing now:
FAIL src/tests/mainnet/routes.ts > Integration Tests - mainnet > utils endpoints > [utils/txs/evaluate/utxos with additional utxo set (JSON)] - utils/txs/evaluate/utxos
(Please, also look at the Preview and Preprod runs.)
In general we need to merge the current main, because on your old main, the GitHub workflow was marked to always pass. Please see this PR by @vladimirvolek for more context:
There was a problem hiding this comment.
BTW, you can reproduce this locally by running:
nix run -L .#internal.x86_64-linux.blockfrost-tests-preview
It will ask you for a few env. variables, like Dolos URL or node socket path, or a funded tADA wallet etc., but once you provide them, it will work the same as on CI.
There was a problem hiding this comment.
And you can easily run a decent Dolos with run-dolos-preview inside the devshell.
But since you don’t need to test any Dolos integration, you can just as well comment it out!
There was a problem hiding this comment.
Sadly, the first approach was the correct one in this PR. I returned back to the original approach with improvements. I've added extensive commenting, but summary here:
We use HashMaps for both sides (Rust & Haskell):
node_utxo_map: on-chain UTxOs keyed by(tx_hash, index)additional_deduped: user-provided UTxOs, also keyed the same way
Conflicts (same TxIn, different TxOut) are rejected with a 400 error before merging. Identical duplicates are silently dropped. Only non-overlapping additional UTxOs survive. User provided data is least important, loses against the node provided data always in conflict.
I should underline, we don't have mempool integration yet (Ogmois does use mempool + chain)
On the Haskell side: the external evaluator receives CBOR-encoded UTxOs which it deserializes into a Map (specifically Data.Map.fromList), so even if there were duplicates in the vector, last entry would win. But we ensure there are none.
in ac3a652.
| @@ -0,0 +1,310 @@ | |||
| use bf_common::errors::AppError; | |||
There was a problem hiding this comment.
Hmmm, I'm thinking perhaps we should make a small PR on main, unrelated to this one, where we move the testgen.rs code to this new mini crate. This way we'll be able to better catch conflicts if something changes there. Now it's easy for the two implementations to diverge, and during conflict resolution just "accept ours" since we deleted the original file in this PR. Just a thought.
I will check how they actually differ before the merge of this one!
There was a problem hiding this comment.
Do you mean, eagerly doing this crate's refactoring in the main? If so I can do that.
| } | ||
| } | ||
|
|
||
| impl<'de> Deserialize<'de> for DeserializationErrorData { |
There was a problem hiding this comment.
Hmmmm, you're deriving Serialize above:
blockfrost-platform/crates/tx_evaluator/src/model/api.rs
Lines 184 to 185 in fe4dd66
… so after this generic serialization, it ends up as JSON object with 6 fields.
But here, it seems like you're deserializing a single string?
So serializer is not a inversion of deserializer, and they don’t match? I think in general JSON codecs should be invertible, or it’s surprising.
There was a problem hiding this comment.
You're right that Serialize and Deserialize are not invertible here. The custom Deserialize impl accepts a single string (from our internal deserialization error path) and expands it into all 6 era fields via conway_only(), while Serialize (derived) outputs the full 6-field JSON object matching the Ogmios response format. This asymmetry is intentional: Ogmios returns all 6 era fields with era-specific error messages, so we replicate that structure for API consumers. But since we only decode in Conway era, on the deserialization side we just take a single string and copy it across all eras.
Added a doc comment in db94c15.
|
|
||
| utxos.insert(TxoRef::from(&multi_era_in), EraCbor::from(multi_era_out)); | ||
| } | ||
| let mut node = node_pool.get().await?; |
There was a problem hiding this comment.
Hmmm. No, no, wait, you're acquiring a second node connection from the pool here. There first one is acquired here:
And in that function you later call this one, which acquires the second one:
It can lead to exhaustion, let's have a single node connection for one request.
There was a problem hiding this comment.
Let's move this conversation into to sub-PR: https://github.com/blockfrost/blockfrost-platform/pull/490/files#r2980627802
It's commented as a to-do there:
// TODO: This evaluator fetches genesis_config_and_pp() from the node on every
// request. It should use ChainConfigWatch to read cached chain config instead.
// Deferred because this native evaluator is currently unused (pallas-validate
// results differ from the Haskell ledger / Ogmios).
| convert_bigint(&sys_start.year)? as i32, | ||
| sys_start.day_of_year as u32, | ||
| ) | ||
| .expect("Invalid system start date"); |
There was a problem hiding this comment.
Hmmm, this is called in convert_protocol_param, which is called in evaluate_tx, so for each request. Is this expect here okay? It’s not only on launch, but potentially for every request.
| let conway_pp: ConwayProtParams = ConwayProtParams { | ||
| minfee_a: pp.minfee_a.unwrap() as u32, | ||
| minfee_b: pp.minfee_b.unwrap() as u32, | ||
| max_block_body_size: pp.max_block_body_size.unwrap() as u32, |
There was a problem hiding this comment.
There's a lot of panicking unwrap()s here on potentially None values.
And yet the returned type is Result<_,_>.
Can you say more?
| script_failures, | ||
| key, | ||
| "noCostModelForLanguage", | ||
| json!(missing_models[0]), |
There was a problem hiding this comment.
I'm almost sure I asked about this already, but have forgotten the answer. Why is it only the first one?
In this case, if this is absolutely correct, can you add an XXX: comment near it? So that it’s always obvious in the future.
There was a problem hiding this comment.
@michalrus I replied, but kept them in pending for a while ^^
|
|
||
| utxos.insert(TxoRef::from(&multi_era_in), EraCbor::from(multi_era_out)); | ||
| } | ||
| let mut node = node_pool.get().await?; |
There was a problem hiding this comment.
Let's move this conversation into to sub-PR: https://github.com/blockfrost/blockfrost-platform/pull/490/files#r2980627802
It's commented as a to-do there:
// TODO: This evaluator fetches genesis_config_and_pp() from the node on every
// request. It should use ChainConfigWatch to read cached chain config instead.
// Deferred because this native evaluator is currently unused (pallas-validate
// results differ from the Haskell ledger / Ogmios).
| script_failures, | ||
| key, | ||
| "noCostModelForLanguage", | ||
| json!(missing_models[0]), |
| @@ -0,0 +1,310 @@ | |||
| use bf_common::errors::AppError; | |||
There was a problem hiding this comment.
Do you mean, eagerly doing this crate's refactoring in the main? If so I can do that.
crates/tx_evaluator/src/model/api.rs
Outdated
|
|
||
| /// If language is native, the json structure can be one of cbor or json | ||
| /// If language is plutus, only cbor field is expected | ||
| /// "script": { |
crates/gateway/Cargo.toml
Outdated
| [dev-dependencies] | ||
| reqwest.workspace = true | ||
| rstest.workspace = true | ||
| tokio-tungstenite.workspace = true |
There was a problem hiding this comment.
Yeah, I didn't notice this difference between CI and my PR. Tho:
> nix develop --command cargo shear
warning: Git tree '/home/sefa/dev/blockfrost-platform' is dirty
shear/misplaced_dependency
× misplaced dependency `reqwest`
╭─[crates/gateway/Cargo.toml:25:1]
24 │ chrono = { workspace = true, features = ["serde"] }
25 │ reqwest.workspace = true
· ───┬───
· ╰── only used in dev targets
26 │ blockfrost.workspace = true
╰────
help: move this dependency to `[dev-dependencies]`
shear/misplaced_dependency
× misplaced dependency `tokio-tungstenite`
╭─[crates/gateway/Cargo.toml:14:1]
13 │ futures-util.workspace = true
14 │ tokio-tungstenite.workspace = true
· ────────┬────────
· ╰── only used in dev targets
15 │ tungstenite.workspace = true
╰────
help: move this dependency to `[dev-dependencies]`
| }; | ||
| use reqwest::Method; | ||
|
|
||
| /// |
There was a problem hiding this comment.
This will produce an empty line. Removed in f71720d.
| } | ||
| } | ||
|
|
||
| impl<'de> Deserialize<'de> for DeserializationErrorData { |
There was a problem hiding this comment.
You're right that Serialize and Deserialize are not invertible here. The custom Deserialize impl accepts a single string (from our internal deserialization error path) and expands it into all 6 era fields via conway_only(), while Serialize (derived) outputs the full 6-field JSON object matching the Ogmios response format. This asymmetry is intentional: Ogmios returns all 6 era fields with era-specific error messages, so we replicate that structure for API consumers. But since we only decode in Conway era, on the deserialization side we just take a single string and copy it across all eras.
Added a doc comment in db94c15.
| if version != 6 { | ||
| Err(BlockfrostError::conflicting_ogmios_version()) | ||
| } else { | ||
| let tx_cbor = binary_or_hex_heuristic(request.transaction.cbor.as_bytes()); |
There was a problem hiding this comment.
@michalrus Yes true, not Ogmios related. Removed in 69f459d.
| pallas-addresses = { git = "https://github.com/blockfrost/pallas.git", tag = "blockfrost-platform-0.0.3-alpha6" } | ||
| pallas-codec = { git = "https://github.com/blockfrost/pallas.git", tag = "blockfrost-platform-0.0.3-alpha6" } | ||
| pallas-crypto = { git = "https://github.com/blockfrost/pallas.git", tag = "blockfrost-platform-0.0.3-alpha6" } | ||
| pallas-hardano = { git = "https://github.com/blockfrost/pallas.git", tag = "blockfrost-platform-0.0.3-alpha6" } |
| @@ -37,5 +37,7 @@ | |||
| "/epochs/latest/parameters", | |||
| "/genesis", | |||
| "/addresses/{address}/transactions", | |||
| "/addresses/{address}/utxos" | |||
| "/addresses/{address}/utxos", | |||
| "/utils/txs/evaluate", | |||
There was a problem hiding this comment.
Sadly, the first approach was the correct one in this PR. I returned back to the original approach with improvements. I've added extensive commenting, but summary here:
We use HashMaps for both sides (Rust & Haskell):
node_utxo_map: on-chain UTxOs keyed by(tx_hash, index)additional_deduped: user-provided UTxOs, also keyed the same way
Conflicts (same TxIn, different TxOut) are rejected with a 400 error before merging. Identical duplicates are silently dropped. Only non-overlapping additional UTxOs survive. User provided data is least important, loses against the node provided data always in conflict.
I should underline, we don't have mempool integration yet (Ogmois does use mempool + chain)
On the Haskell side: the external evaluator receives CBOR-encoded UTxOs which it deserializes into a Map (specifically Data.Map.fromList), so even if there were duplicates in the vector, last entry would win. But we ensure there are none.
in ac3a652.
For example: The API documentation example incorrectly uses "cpu" without "EvaluationResult" wrapper, but the actual Ogmios v5.6 JSON schema (ogmios.wsp.json) and source code use "steps" with "EvaluationResult".
|
|
||
| if !even_length || contains_non_hex { | ||
| xs.to_vec() | ||
| use base64::{Engine as _, engine::general_purpose}; |
There was a problem hiding this comment.
@michalrus Please see if this change doesn't belong here (I can separate). This is needed since hosted Blockfrost API accepts base64 in evaluate endpoint.
Resolves #306
Context
The goal of this PR is to implement tx evaluation APIs while trying to stay compatible to the Blockfrost.io version, which are:
/utils/txs/evaluate/utils/txs/evaluate/utxosWhat is added
evaluateprotocol_paramsandgenesis_configpallas_validatehandlingpallas(incomplete)What is changed
cargo.lockfile contentConsiderations
evaluatewill be built by default except windows and linux-aarch64 platforms (handled inbuild.rs).evaluateenables tx evaluation endpoints. It exists sincepallat-validatecrate isn't building for windows-all and linux-aarch64 at the moment.Please run
cargo cleanto make sure everything is in a fresh state before compiling.Related PRs
Haskell part: Implemented eval tx stream testgen-hs#3
Pallas: Lots of fixes and updated to the latest node version txpipe/pallas#624