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

Updates/merge cuttlefish #1007

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Updates/merge cuttlefish #1007

wants to merge 4 commits into from

Conversation

dhedey
Copy link
Contributor

@dhedey dhedey commented Oct 9, 2024

Summary

A PR to update the engine in the node to align with Cuttlefish.

This PR just gets the bottlenose node compiling/syncing with the Cuttlefish engine code. It does not yet implement any explicit cuttlefish functionality in the node, and there are a number of todo!s left in which will be fixed in later work.

Testing

The testing strategy is that:

  • Existing tests pass
  • Can sync a stokenet node with a fresh ledger

(Need to check that our backwards compatibility test suite works 👍)

@dhedey dhedey changed the base branch from main to develop October 9, 2024 01:33
Copy link

github-actions bot commented Oct 9, 2024

Phylum Report Link

@codecov-commenter
Copy link

codecov-commenter commented Oct 14, 2024

Codecov Report

Attention: Patch coverage is 48.88287% with 755 lines in your changes missing coverage. Please review.

Project coverage is 43.1%. Comparing base (93bc7ea) to head (fb722dc).

Files with missing lines Patch % Lines
...-server/src/core_api/handlers/transaction_parse.rs 0.0% 101 Missing ⚠️
...e-rust/state-manager/src/transaction/validation.rs 2.3% 83 Missing ⚠️
...er/src/mempool/pending_transaction_result_cache.rs 25.0% 57 Missing ⚠️
...-rust/state-manager/src/transaction/preparation.rs 76.8% 37 Missing ⚠️
...erver/src/core_api/handlers/stream_transactions.rs 0.0% 27 Missing ⚠️
...rust/state-manager/src/jni/transaction_preparer.rs 0.0% 27 Missing ⚠️
core-rust/state-manager/src/types.rs 62.1% 25 Missing ⚠️
...src/protocol/protocol_updates/protocol_updaters.rs 11.5% 23 Missing ⚠️
...e_api/conversions/substates/access_rules_module.rs 0.0% 22 Missing ⚠️
...r/src/engine_state_api/conversions/access_rules.rs 0.0% 20 Missing ⚠️
... and 75 more
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             develop   #1007     +/-   ##
===========================================
- Coverage       43.5%   43.1%   -0.4%     
+ Complexity      4571    4570      -1     
===========================================
  Files           1731    1730      -1     
  Lines          53028   52805    -223     
  Branches        1514    1515      +1     
===========================================
- Hits           23072   22781    -291     
- Misses         29477   29545     +68     
  Partials         479     479             
Flag Coverage Δ
rust 43.1% <48.8%> (-0.4%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...core-api-server/src/core_api/conversions/common.rs 0.0% <ø> (ø)
...ore-api-server/src/core_api/conversions/context.rs 0.0% <ø> (ø)
...core-api-server/src/core_api/conversions/errors.rs 0.0% <ø> (ø)
...i-server/src/core_api/conversions/keys_and_sigs.rs 0.0% <ø> (ø)
...re-api-server/src/core_api/conversions/numerics.rs 45.5% <ø> (ø)
...-api-server/src/core_api/conversions/pagination.rs 0.0% <ø> (ø)
core-rust/core-api-server/src/core_api/errors.rs 0.0% <ø> (ø)
...ts/state_account_all_fungible_resource_balances.rs 0.0% <ø> (ø)
...pi/handlers/lts/state_account_deposit_behaviour.rs 0.0% <ø> (ø)
...api/handlers/lts/state_account_resource_balance.rs 0.0% <ø> (ø)
... and 153 more

@dhedey dhedey marked this pull request as ready for review October 14, 2024 19:09
Copy link

github-actions bot commented Oct 14, 2024

Docker tags
docker.io/radixdlt/private-babylon-node:pr-1007
docker.io/radixdlt/private-babylon-node:1975a35004
docker.io/radixdlt/private-babylon-node:sha-1975a35

Copy link

sonarcloud bot commented Oct 14, 2024

@@ -140,6 +140,10 @@ pub fn to_api_substate_id(
SubstateType::BootLoaderModuleFieldKernelBoot,
models::PartitionKind::Field,
),
TypedSubstateKey::BootLoader(TypedBootLoaderSubstateKey::BootLoaderField(
BootLoaderField::TransactionValidationConfiguration,
)) => todo!(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm that we've got a good way to track all these todos. :)

@@ -711,7 +704,7 @@ pub fn to_api_costing_parameters(
xrd_usd_price: to_api_decimal(&engine_costing_parameters.usd_price),
xrd_storage_price: to_api_decimal(&engine_costing_parameters.state_storage_price),
xrd_archive_storage_price: to_api_decimal(&engine_costing_parameters.archive_storage_price),
tip_percentage: to_api_u16_as_i32(transaction_costing_parameters.tip_percentage),
tip_percentage: tip_percentage_decimal.try_into().unwrap_or(i32::MAX),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we unwrap?

@@ -217,11 +204,11 @@ impl From<MempoolAddError> for MempoolAddErrorJava {
fn from(err: MempoolAddError) -> Self {
match err {
MempoolAddError::PriorityThresholdNotMet {
min_tip_basis_points_required: min_tip_percentage_required,
tip_basis_points: tip_percentage,
} => MempoolAddErrorJava::PriorityThresholdNotMet {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: remove these confusing renames?

}
MempoolAddError::PriorityThresholdNotMet {
min_tip_basis_points_required: min_tip_percentage_required,
tip_basis_points: tip_percentage,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: remove these confusing renames?

write!(f, "Priority Threshold not met. There is no known tip to guarantee mempool submission.")
}
Some(min_tip_percentage_required) => {
write!(f, "Priority Threshold not met: tip is {tip_percentage} while min tip required {min_tip_percentage_required}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tip => basis point

.transactions
.remove(0);
let ProtocolUpdateTransactionDetails::FlashV1Transaction(flash) = validator_fee_fix;
let ProtocolUpdateTransaction::FlashTransactionV1(flash) = validator_fee_fix else {
panic!();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a message here?

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.

3 participants