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

CSUB-1379: Make danger check more robust #528

Merged
merged 3 commits into from
Nov 21, 2024

Conversation

atodorov
Copy link
Contributor

@atodorov atodorov commented Nov 20, 2024

Make the check-for-changes-in-epoch-duration.sh more robust so that it doesn't cause so much false positives.

They are still possible (see commit log & the examples below) however the more common case where we're adding something extra in the vicinity of the constants that we check for will no longer trigger.

Notes:

  • originally opened against a modified base branch in order for the issue to trigger & collect logs
  • already proposed for dev

@atodorov
Copy link
Contributor Author

atodorov commented Nov 20, 2024

TEST RESULTS:

  1. A false positive before any changes are introduced -> https://github.com/gluwa/creditcoin3/actions/runs/11932446096/job/33257418045?pr=528. The original diff context is
diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs
index f0950475..461ff812 100644
--- a/runtime/src/lib.rs
+++ b/runtime/src/lib.rs
@@ -39,11 +39,12 @@ use frame_support::weights::{constants::ParityDbWeight as RuntimeDbWeight, Weigh
 use frame_support::{
     construct_runtime, parameter_types,
     traits::{
-        ConstU128, ConstU32, ConstU8, FindAuthor, InstanceFilter, KeyOwnerProofSystem, OnFinalize,
+        ConstU32, ConstU8, FindAuthor, InstanceFilter, KeyOwnerProofSystem, OnFinalize,
         OnTimestampSet,
     },
     weights::{
-        constants::WEIGHT_REF_TIME_PER_MILLIS, ConstantMultiplier, Weight, WeightToFeeCoefficient,
+        constants::WEIGHT_REF_TIME_PER_MILLIS, constants::WEIGHT_REF_TIME_PER_NANOS,
+        ConstantMultiplier, Weight, WeightToFeeCoefficient,
     },
     PalletId,
 };
@@ -149,6 +150,8 @@ pub const MILLISECS_PER_BLOCK: u64 = prod_or_fast!(15_000, 5_000);
 
 pub const SLOT_DURATION: u64 = MILLISECS_PER_BLOCK;
 
+pub const CENTS: u128 = CTC / 100;
+
 // WARNING: the next should be defined on a single line b/c of
 // the assertions made in .github/check-for-changes-in-epoch-duration.sh
 #[rustfmt::skip]
@@ -326,33 +329,34 @@ impl pallet_balances::Config for Runtime {
     type MaxFreezes = ();
 }
 
-pub const TARGET_FEE_CREDO: Balance = 10_000_000_000_000_000;
+parameter_types! {
+    /// Executing a NO-OP `System::remarks` Extrinsic.
+    pub const ExtrinsicBaseWeight: Weight =
+        Weight::from_parts(WEIGHT_REF_TIME_PER_NANOS.saturating_mul(125_000), 0);
+}
+
 pub struct WeightToCtcFee<T>(PhantomData<T>);
 
 impl<T: frame_system::Config> WeightToFeePolynomial for WeightToCtcFee<T> {
     type Balance = Balance;
 
     fn polynomial() -> frame_support::weights::WeightToFeeCoefficients<Self::Balance> {
-        // Copied from the weights for the lock deal order extrinsic which is what is referenced in cc2
-        let deal_order_weight = Weight::from_parts(30_601_000, 0)
-            .saturating_add(Weight::from_parts(0, 4089))
-            .saturating_add(T::DbWeight::get().reads(1))
-            .saturating_add(T::DbWeight::get().writes(1));
-
-        let base = Balance::from(deal_order_weight.ref_time());
-        let ratio = TARGET_FEE_CREDO / base;
-        let rem = TARGET_FEE_CREDO % base;
-        smallvec::smallvec!(WeightToFeeCoefficient {
-            coeff_integer: ratio,
-            coeff_frac: Perbill::from_rational(rem, base),
-            negative: false,
+        let p = CENTS;
+        let q = 10 * Balance::from(ExtrinsicBaseWeight::get().ref_time());
+        smallvec::smallvec![WeightToFeeCoefficient {
             degree: 1,
-        })
+            negative: false,
+            coeff_frac: Perbill::from_rational(p % q, q),
+            coeff_integer: p / q,
+        }]
     }
 }
 
+pub const TRANSACTION_BYTE_FEE: Balance = 10 * CENTS / 1_000;
+
 parameter_types! {
     pub FeeMultiplier: Multiplier = Multiplier::one();
+    pub const TransactionByteFee: Balance = TRANSACTION_BYTE_FEE;
 }
 
 impl pallet_transaction_payment::Config for Runtime {
@@ -360,7 +364,7 @@ impl pallet_transaction_payment::Config for Runtime {
     type OnChargeTransaction = CurrencyAdapter<Balances, ()>;
     type OperationalFeeMultiplier = ConstU8<1u8>;
     type WeightToFee = WeightToCtcFee<Runtime>;
-    type LengthToFee = ConstantMultiplier<u128, ConstU128<1_500_000_000u128>>;
+    type LengthToFee = ConstantMultiplier<u128, TransactionByteFee>;
     type FeeMultiplierUpdate = ConstFeeMultiplier<FeeMultiplier>;
 }
 
diff --git a/runtime/src/version.rs b/runtime/src/version.rs
index f428a161..9ce651f3 100644
--- a/runtime/src/version.rs
+++ b/runtime/src/version.rs
@@ -7,7 +7,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
     spec_name: create_runtime_str!("creditcoin3"),
     impl_name: create_runtime_str!("creditcoin3"),
     authoring_version: 3,
-    spec_version: 37,
+    spec_version: 39,
     impl_version: 0,
     apis: RUNTIME_API_VERSIONS,
     transaction_version: 2,
  1. No trigger for the original diff under runtime/ https://github.com/gluwa/creditcoin3/actions/runs/11933097551/job/33259446193?pr=528

  2. Script reports FAIL with the following change, as expected, https://github.com/gluwa/creditcoin3/actions/runs/11933156764/job/33259634784?pr=528

diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs
index 461ff812..07cd4a70 100644
--- a/runtime/src/lib.rs
+++ b/runtime/src/lib.rs
@@ -146,7 +146,7 @@ macro_rules! prod_or_fast {
     };
 }
 
-pub const MILLISECS_PER_BLOCK: u64 = prod_or_fast!(15_000, 5_000);
+pub const MILLISECS_PER_BLOCK: u64 = prod_or_fast!(15_000, 6_000);
 
 pub const SLOT_DURATION: u64 = MILLISECS_PER_BLOCK;
  1. Script reports FAIL with the following change, as expected, https://github.com/gluwa/creditcoin3/actions/runs/11933206332/job/33259787782?pr=528
diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs
index 461ff812..ff301de8 100644
--- a/runtime/src/lib.rs
+++ b/runtime/src/lib.rs
@@ -155,7 +155,7 @@ pub const CENTS: u128 = CTC / 100;
 // WARNING: the next should be defined on a single line b/c of
 // the assertions made in .github/check-for-changes-in-epoch-duration.sh
 #[rustfmt::skip]
-const BLOCKS_FOR_FASTER_EPOCH: u32 = if cfg!(feature = "devnet") { 2 * HOURS } else { 15 };
+const BLOCKS_FOR_FASTER_EPOCH: u32 = if cfg!(feature = "devnet") { 2 * HOURS } else { 10 };
 
 pub const EPOCH_DURATION_IN_BLOCKS: u32 = prod_or_fast!(12 * HOURS, BLOCKS_FOR_FASTER_EPOCH);
  1. False positive triggered for BLOCKS_FOR_FASTER_EPOCH, as described in the commit log, https://github.com/gluwa/creditcoin3/actions/runs/11933276483/job/33259998480?pr=528 (although the current change itself is something that is of concern too)
diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs
index 461ff812..ed2e2b28 100644
--- a/runtime/src/lib.rs
+++ b/runtime/src/lib.rs
@@ -157,7 +157,7 @@ pub const CENTS: u128 = CTC / 100;
 #[rustfmt::skip]
 const BLOCKS_FOR_FASTER_EPOCH: u32 = if cfg!(feature = "devnet") { 2 * HOURS } else { 15 };
 
-pub const EPOCH_DURATION_IN_BLOCKS: u32 = prod_or_fast!(12 * HOURS, BLOCKS_FOR_FASTER_EPOCH);
+pub const EPOCH_DURATION_IN_BLOCKS: u32 = prod_or_fast!(12 * HOURS, 20);
 
 parameter_types! {
     pub const EpochDuration: u64 = EPOCH_DURATION_IN_BLOCKS as u64; // Q: how long to make an epoch
  1. Script reports FAIL with the following change, as expected, https://github.com/gluwa/creditcoin3/actions/runs/11933366571/job/33260268203?pr=528
diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs
index 461ff812..9f3dab10 100644
--- a/runtime/src/lib.rs
+++ b/runtime/src/lib.rs
@@ -160,7 +160,7 @@ const BLOCKS_FOR_FASTER_EPOCH: u32 = if cfg!(feature = "devnet") { 2 * HOURS } e
 pub const EPOCH_DURATION_IN_BLOCKS: u32 = prod_or_fast!(12 * HOURS, BLOCKS_FOR_FASTER_EPOCH);
 
 parameter_types! {
-    pub const EpochDuration: u64 = EPOCH_DURATION_IN_BLOCKS as u64; // Q: how long to make an epoch
+    pub const EpochDuration: u64 = 6 * HOURS; // Q: how long to make an epoch
     pub const ExpectedBlockTime: u64 = MILLISECS_PER_BLOCK; // Q: block time
 }
  1. Script reports FAIL with the following change, as expected, https://github.com/gluwa/creditcoin3/actions/runs/11933402221/job/33260376878?pr=528 (however it is the check for MILLISECS_PER_BLOCK which triggers first)
diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs
index 461ff812..13c7bafb 100644
--- a/runtime/src/lib.rs
+++ b/runtime/src/lib.rs
@@ -148,7 +148,7 @@ macro_rules! prod_or_fast {
 
 pub const MILLISECS_PER_BLOCK: u64 = prod_or_fast!(15_000, 5_000);
 
-pub const SLOT_DURATION: u64 = MILLISECS_PER_BLOCK;
+pub const SLOT_DURATION: u64 = 2 * MILLISECS_PER_BLOCK;
 
 pub const CENTS: u128 = CTC / 100;

@atodorov atodorov force-pushed the testing/CSUB-1379-make-danger-check-more-robust branch 5 times, most recently from 6684bf3 to 6696f2d Compare November 20, 2024 12:29
this will print only the messages that we want, not execution of every
command in the script, which should make it easier to read the output!
- update wording to be more precise wrt what this script is doing
- introduce colors for visual separation
- exclude unmodified lines from the diff context by searching for lines
  which explicitly start with + or -, meaning added/removed lines. This
  will prevent the script to trigger when adding/modifying other
  constants in the vicinity of the ones that we check for!

- as an extra measure search only for modified lines which also contain
  the word "const" inside of them! This will prevent the script to
  trigger when modifying function calls which merely use the value of
  the constants that we check for!

- as more extra measure examine diff only on the runtime/ directory to
  avoid triggering by changes in this script itself or other files which
  may be referencing a constant name, e.g. text documentation.

NOTE: the script can still trigger with a false positive if for example
you modify a definition for a different constant, whose value is an
expression consuming the constants we check for.

For example:

-const CTC_REWARD_PER_BLOCK: Balance = 2 * CTC;
+const CTC_REWARD_PER_BLOCK: Balance = 2 * CTC * EPOCH_DURATION_IN_BLOCKS;

will trigger the script!

Improve by diffing only runtime/
@atodorov atodorov changed the base branch from testing/CSUB-1379-bogus-base-branch to dev November 20, 2024 12:31
@atodorov atodorov force-pushed the testing/CSUB-1379-make-danger-check-more-robust branch from 6696f2d to 8c6bd21 Compare November 20, 2024 12:32
@atodorov atodorov marked this pull request as ready for review November 20, 2024 12:32
Copy link

For full LLVM coverage report click here!

Copy link
Contributor

@beqaabu beqaabu left a comment

Choose a reason for hiding this comment

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

neat

@atodorov atodorov merged commit e570d4a into dev Nov 21, 2024
50 of 53 checks passed
@atodorov atodorov deleted the testing/CSUB-1379-make-danger-check-more-robust branch November 21, 2024 08:50
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.

4 participants