[agg_v2] Adding aggregator_v2.move with sequential (fallback) logic #10397

igor-aptos · 2023-10-04T18:15:02Z

Self-contained logic that can be landed on main, and so ecosystem PRs can land independently of the rest of the aggregator changes

once aggregator backend execution logic lands, this will be a fallback logic used until separate aggregator execution flag is turned on.

Copied from aggregators_v2 branch

Description

Test Plan

igor-aptos · 2023-10-04T18:37:18Z

aptos-move/aptos-native-interface/src/context.rs

@@ -123,4 +123,9 @@ impl<'a, 'b, 'c, 'd> SafeNativeContext<'a, 'b, 'c, 'd> {
    pub fn aggregator_snapshots_enabled(&self) -> bool {
        self.get_feature_flags().is_aggregator_snapshots_enabled()
    }
+
+    /// TODO: decide if we want to have two flags or merge with aggregator_snapshots_enabled.


we are introducing new native functions that need to be gated.

since aggregator_snapshots_enabled was never enabled, we can either:

decide not to enable it before v1.8, and combine aggregator_api_enabled into it.

if we want to enable in point release for v1.7, we would need to add a new flag instead.

vusirikala · 2023-10-12T17:16:26Z

aptos-move/framework/aptos-framework/sources/aggregator_v2/aggregator_v2.move

+    ///
+    /// Currently supported types for Element are u64 and u128.
+    /// EAGGREGATOR_ELEMENT_TYPE_NOT_SUPPORTED raised if called with a different type.
+    public native fun create_unbounded_aggregator<IntElement: copy + drop>(): Aggregator<IntElement>;


Does this have to be a native function? It can simply call create_aggregator with an appropriate max_value right?

I don't think it can be done, because the type is generic.

i.e. something like this doesn't compile:

public fun create_unbounded_aggregator<IntElement: copy + drop>(): Aggregator<IntElement> { if (type_info::type_of<IntElement>() == type_info::type_of<u64>()) { create_aggregator(MAX_U64) } else if (type_info::type_of<IntElement>() == type_info::type_of<u128>()) { create_aggregator(MAX_U128) } else { abort error::invalid_argument(EUNSUPPORTED_AGGREGATOR_TYPE); } }

this compiles:
if (type_info::type_of<Element>() == type_info::type_of<u64>()) {

this doesn't:
return create_aggregator(MAX_U64)
when return type of the function is : Aggregator

Compiler could probably infer something, but even in C++/rust not sure that would pass. Not sure if there is a different way to write this.

vusirikala · 2023-10-12T17:31:14Z

aptos-move/framework/aptos-framework/sources/aggregator_v2/aggregator_v2.move

+    }
+
+    #[test(fx = @std)]
+    public fun test_string_concat2(fx: &signer) {


Do these test cases succeed now? They weren't successful when I merged the aggregator snapshot changes in main.

there was a bug in how features were handled in the harness, that was fixed. I'll recheck , but I think they pass now

seems like I imagined something, they still don't work.

vusirikala · 2023-10-12T18:15:38Z

aptos-move/framework/src/natives/aggregator_natives/aggregator_v2.rs

@@ -39,94 +50,418 @@ fn is_string_type(context: &SafeNativeContext, type_arg: &Type) -> SafeNativeRes
    Ok(false)
 }

+/// Given the list of native function arguments and a type, returns a tuple of its


Nit: We are only giving one native function argument.

vusirikala · 2023-10-13T01:34:28Z

aptos-move/framework/src/natives/aggregator_natives/aggregator_v2.rs

+    ) -> SafeNativeResult<SnapshotValue> {
+        match self {
+            SnapshotType::U128 => Ok(SnapshotValue::Integer(safely_pop_arg!(args, u128))),
+            SnapshotType::U64 => Ok(SnapshotValue::Integer(safely_pop_arg!(args, u64) as u128)),


Note for the future: SnapshotValue is replaced by DelayedFieldValue right?

no - SnapshotValue encompasses types that AggregatorSnapshot can take

DelayedFieldValue is list of Delayed Fields (today aggregator, snapshot delta and snapshot derived. tomorrow maybe more).

this code is identical on aggregators_v2 branch - https://github.com/aptos-labs/aptos-core/blob/aggregators_v2/aptos-move/framework/src/natives/aggregator_natives/aggregator_v2.rs#L143

vusirikala · 2023-10-13T01:50:16Z

aptos-move/framework/src/natives/aggregator_natives/aggregator_v2.rs

+}
+
+impl SnapshotToStringFormula {
+    pub fn apply_to(&self, base: u128) -> Vec<u8> {


What is we are deriving the snapshot from aggregator_u64? Are we still going to treat u64 as u128 when creating string snapshot?

we always go to u128, and cast to appropriate type when return to move, for code simplicity.

And all this is identical on aggregator_v2 branch - https://github.com/aptos-labs/aptos-core/blob/aggregators_v2/aptos-move/aptos-aggregator/src/types.rs#L322

vusirikala · 2023-10-13T01:52:09Z

aptos-move/framework/src/natives/aggregator_natives/aggregator_v2.rs

+// TODO: aggregator_v2 branch will introduce these in different places in code
+
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub enum SnapshotValue {


We are going to land aggregator_v2 branch on main in a week anyway. What are we benefitting by landing these stuff here in main now, and changing them again after landing aggregator_v2?

vgao1996 · 2023-10-13T20:56:56Z

aptos-move/aptos-gas-schedule/src/gas_schedule/aptos_framework.rs

@@ -201,11 +201,17 @@ crate::gas_schedule::macros::define_gas_parameters!(
        [aggregator_destroy_base: InternalGas, "aggregator.destroy.base", 10000],
        [aggregator_factory_new_aggregator_base: InternalGas, "aggregator_factory.new_aggregator.base", 10000],

+        [aggregator_v2_snapshot_base: InternalGas, {11.. => "aggregator_v2.snapshot.base"}, 6000],


Just a reminder to fix the gas feature version here

zekun000 · 2023-10-14T03:42:44Z

aptos-move/framework/aptos-framework/sources/aggregator_v2/aggregator_v2.move

+    ///
+    /// Currently supported types for Element are u64 and u128.
+    /// EAGGREGATOR_ELEMENT_TYPE_NOT_SUPPORTED raised if called with a different type.
+    public native fun create_aggregator<IntElement: copy + drop>(max_value: IntElement): Aggregator<IntElement>;


if we only support u64 and u128, I think it's better to write constructors explicitly and leave other functions generic

fun create_u64_aggregator(max_value: u64): Aggregator<u64>;

why?

usage here seems just fine - https://github.com/aptos-labs/aptos-core/pull/9972/files#diff-7026c328622536165825a6f5dcaf13764bd475489ecba30ba4716dfdc4acc1e9

and set of types we support is something we can expand on in the future.

Self-contained logic that can be landed on main, and so ecosystem PRs can land independently of the rest of the aggregator changes Copied from aggregators_v2 branch Fixing tests and more comments gas fixes and limit to string length

georgemitenkov · 2023-10-18T21:14:50Z

aptos-move/framework/aptos-framework/sources/aggregator_v2/aggregator_v2.move

+    const EAGGREGATOR_OVERFLOW: u64 = 1;
+
+    /// The value of aggregator underflows (goes below zero). Raised by uncoditional sub() call
+    const EAGGREGATOR_UNDERFLOW: u64 = 2;


@igor-aptos what was 3 and 4?

I think that was from v1 aggregators, to disambiguate.

though I see only 3 there now:

const ENOT_SUPPORTED: u64 = 3;

because rest are consistent:

/// The value of aggregator overflows. Raised by native code. const EAGGREGATOR_OVERFLOW: u64 = 1; /// The value of aggregator underflows (goes below zero). Raised by native code. const EAGGREGATOR_UNDERFLOW: u64 = 2;

ENOT_SUPPORTED is never used I think

georgemitenkov · 2023-10-18T21:16:00Z

aptos-move/framework/aptos-framework/sources/aggregator_v2/aggregator_v2.move

+
+    /// Creates new aggregator, with given 'max_value'.
+    ///
+    /// Currently supported types for Element are u64 and u128.


Element --> IntElement?

georgemitenkov · 2023-10-18T21:17:53Z

aptos-move/framework/aptos-framework/sources/aggregator_v2/aggregator_v2.move

+    public native fun try_add<IntElement>(aggregator: &mut Aggregator<IntElement>, value: IntElement): bool;
+
+    // Adds `value` to aggregator, uncoditionally.
+    // If addition would exceed the max_value, EAGGREGATOR_OVERFLOW exception will be thrown


nit: missing . in docstring

georgemitenkov · 2023-10-18T21:18:02Z

aptos-move/framework/aptos-framework/sources/aggregator_v2/aggregator_v2.move

+    public native fun try_sub<IntElement>(aggregator: &mut Aggregator<IntElement>, value: IntElement): bool;
+
+    // Adds `value` to aggregator, uncoditionally.
+    // If subtraction would result in a negative value, EAGGREGATOR_UNDERFLOW exception will be thrown


Also, adds --> subtracts

georgemitenkov · 2023-10-18T21:20:49Z

aptos-move/framework/aptos-framework/sources/aggregator_v2/aggregator_v2.move

-    // }
+    #[test]
+    #[expected_failure(abort_code = 0x030009, location = Self)]
+    public fun test_copy_not_yet_supported() {


why public functions? here and before/after as well

georgemitenkov · 2023-10-18T21:33:29Z

aptos-move/framework/src/natives/aggregator_natives/aggregator_v2.rs

+            Ok((id, limit))
+        },
+        Type::U64 => {
+            // Get aggregator information and a value to add.


georgemitenkov · 2023-10-18T21:36:02Z

aptos-move/framework/src/natives/aggregator_natives/aggregator_v2.rs

+                    },
+                )])))
+            },
+            // ty_arg cannot be Integer, if value is String


what is ty_arg here?

georgemitenkov · 2023-10-18T21:47:17Z

aptos-move/framework/src/natives/aggregator_natives/aggregator_v2.rs

+) -> SafeNativeResult<SmallVec<[Value; 1]>> {
+    abort_if_not_enabled!(context);
+
+    debug_assert_eq!(args.len(), 0);


Should we assert we have a single type arg

georgemitenkov · 2023-10-18T21:54:43Z

aptos-move/framework/src/natives/aggregator_natives/aggregator_v2.rs

+    let result_value = {
+        let math = BoundedMath::new(agg_max_value);
+        match math.unsigned_subtract(agg_value, input) {
+            Ok(sum) => {


sum --> diff/difference?

georgemitenkov · 2023-10-18T22:13:55Z

aptos-move/framework/src/natives/aggregator_natives/aggregator_v2.rs

+
+    let prefix = string_to_bytes(safely_pop_arg!(args, Struct))?;
+
+    if prefix.len() + suffix.len() > CONCAT_PREFIX_AND_SUFFIX_MAX_LENGTH {


I think I asked on some other PR before, this sum is always < max u64

github-actions · 2023-10-19T16:33:11Z

✅ Forge suite `compat` success on `aptos-node-v1.6.2` ==> `aa4be7dd91eb31441e36c7c4414118668b4e79a1`

Compatibility test results for aptos-node-v1.6.2 ==> aa4be7dd91eb31441e36c7c4414118668b4e79a1 (PR)
1. Check liveness of validators at old version: aptos-node-v1.6.2
compatibility::simple-validator-upgrade::liveness-check : committed: 3346 txn/s, latency: 7311 ms, (p50: 7800 ms, p90: 10200 ms, p99: 11100 ms), latency samples: 167300
2. Upgrading first Validator to new version: aa4be7dd91eb31441e36c7c4414118668b4e79a1
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1829 txn/s, latency: 15913 ms, (p50: 18600 ms, p90: 22300 ms, p99: 22600 ms), latency samples: 93320
3. Upgrading rest of first batch to new version: aa4be7dd91eb31441e36c7c4414118668b4e79a1
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1292 txn/s, latency: 16900 ms, (p50: 19300 ms, p90: 22500 ms, p99: 48000 ms), latency samples: 91800
4. upgrading second batch to new version: aa4be7dd91eb31441e36c7c4414118668b4e79a1
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3574 txn/s, latency: 8914 ms, (p50: 8600 ms, p90: 12600 ms, p99: 12900 ms), latency samples: 142960
5. check swarm health
Compatibility test for aptos-node-v1.6.2 ==> aa4be7dd91eb31441e36c7c4414118668b4e79a1 passed
Test Ok

github-actions · 2023-10-19T16:35:29Z

✅ Forge suite `realistic_env_max_load` success on `aa4be7dd91eb31441e36c7c4414118668b4e79a1`

two traffics test: inner traffic : committed: 7666 txn/s, latency: 5093 ms, (p50: 4800 ms, p90: 6600 ms, p99: 9900 ms), latency samples: 3334840
two traffics test : committed: 100 txn/s, latency: 2825 ms, (p50: 2100 ms, p90: 4300 ms, p99: 9000 ms), latency samples: 1800
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.211, avg: 0.198", "QsPosToProposal: max: 0.185, avg: 0.156", "ConsensusProposalToOrdered: max: 0.629, avg: 0.580", "ConsensusOrderedToCommit: max: 0.521, avg: 0.497", "ConsensusProposalToCommit: max: 1.140, avg: 1.077"]
Max round gap was 1 [limit 4] at version 1651562. Max no progress secs was 4.097545 [limit 10] at version 1651562.
Test Ok

github-actions · 2023-10-19T16:48:58Z

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.5.1` ==> `aa4be7dd91eb31441e36c7c4414118668b4e79a1`

Compatibility test results for aptos-node-v1.5.1 ==> aa4be7dd91eb31441e36c7c4414118668b4e79a1 (PR)
Upgrade the nodes to version: aa4be7dd91eb31441e36c7c4414118668b4e79a1
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 4115 txn/s, latency: 5011 ms, (p50: 5000 ms, p90: 7800 ms, p99: 9500 ms), latency samples: 234600
5. check swarm health
Compatibility test for aptos-node-v1.5.1 ==> aa4be7dd91eb31441e36c7c4414118668b4e79a1 passed
Test Ok

igor-aptos requested review from vusirikala, gelash, georgemitenkov, zekun000 and movekevin October 4, 2023 18:15

igor-aptos requested review from saharct, davidiw and wrwg as code owners October 4, 2023 18:15

igor-aptos commented Oct 4, 2023

View reviewed changes

vusirikala reviewed Oct 12, 2023

View reviewed changes

igor-aptos force-pushed the igor/aggregators_v2_on_main branch 3 times, most recently from e854e01 to 475d73f Compare October 13, 2023 00:43

igor-aptos requested a review from vusirikala October 13, 2023 00:44

vusirikala reviewed Oct 13, 2023

View reviewed changes

vusirikala approved these changes Oct 13, 2023

View reviewed changes

vgao1996 reviewed Oct 13, 2023

View reviewed changes

igor-aptos force-pushed the igor/aggregators_v2_on_main branch from 475d73f to c255306 Compare October 13, 2023 22:47

zekun000 reviewed Oct 14, 2023

View reviewed changes

vusirikala requested a review from junkil-park as a code owner October 16, 2023 20:26

igor-aptos force-pushed the igor/aggregators_v2_on_main branch 4 times, most recently from f10fc4a to 57e67a5 Compare October 17, 2023 03:44

igor-aptos added 2 commits October 17, 2023 16:58

[agg_v2] Adding aggregator_v2.move and fallback logic

bb9bc64

Self-contained logic that can be landed on main, and so ecosystem PRs can land independently of the rest of the aggregator changes Copied from aggregators_v2 branch Fixing tests and more comments gas fixes and limit to string length

addressing comments/updates

65b233d

igor-aptos force-pushed the igor/aggregators_v2_on_main branch from 57e67a5 to 65b233d Compare October 17, 2023 23:58

georgemitenkov approved these changes Oct 18, 2023

View reviewed changes

igor-aptos force-pushed the igor/aggregators_v2_on_main branch from 03c003f to c21d104 Compare October 19, 2023 06:37

fix lint

aa4be7d

igor-aptos force-pushed the igor/aggregators_v2_on_main branch from c21d104 to aa4be7d Compare October 19, 2023 16:04

igor-aptos enabled auto-merge (squash) October 19, 2023 16:04

This comment has been minimized.

Sign in to view

igor-aptos merged commit 7ba0e69 into main Oct 19, 2023
60 of 82 checks passed

igor-aptos deleted the igor/aggregators_v2_on_main branch October 19, 2023 16:35

igor-aptos mentioned this pull request Oct 19, 2023

[agg_v2] Add more documentation and unbounded constructor #10516

Closed


		let prefix = string_to_bytes(safely_pop_arg!(args, Struct))?;

		if prefix.len() + suffix.len() > CONCAT_PREFIX_AND_SUFFIX_MAX_LENGTH {

[agg_v2] Adding aggregator_v2.move with sequential (fallback) logic #10397

[agg_v2] Adding aggregator_v2.move with sequential (fallback) logic #10397

Conversation

igor-aptos commented Oct 4, 2023

Description

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Oct 19, 2023

✅ Forge suite compat success on aptos-node-v1.6.2 ==> aa4be7dd91eb31441e36c7c4414118668b4e79a1

github-actions bot commented Oct 19, 2023

✅ Forge suite realistic_env_max_load success on aa4be7dd91eb31441e36c7c4414118668b4e79a1

github-actions bot commented Oct 19, 2023

✅ Forge suite framework_upgrade success on aptos-node-v1.5.1 ==> aa4be7dd91eb31441e36c7c4414118668b4e79a1

✅ Forge suite `compat` success on `aptos-node-v1.6.2` ==> `aa4be7dd91eb31441e36c7c4414118668b4e79a1`

✅ Forge suite `realistic_env_max_load` success on `aa4be7dd91eb31441e36c7c4414118668b4e79a1`

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.5.1` ==> `aa4be7dd91eb31441e36c7c4414118668b4e79a1`