Refactor pass feature status to deserialized packet via packet meta #31549

tao-stones · 2023-05-09T00:46:32Z

Problem

Passing current root_bank's feature status from packet_receiver to ImmutableDeserializedPacket is cumbersome. The plumbing go through many level call stacks (poc #31543).

Another possibility is to add an additional flag to Packet, which is set by root bank feature status at the moment of packet deserialization. So the feature status goes with packet instead of passing multiple layers of calls. It'd also make removing feature code easier when it's fully activated.

Summary of Changes

add round-compute-unit-price flag to Packet
PacketDeserializer uses bank_fork to get root_bank's feature set status

Fixes #

codecov · 2023-05-09T05:03:31Z

Codecov Report

Merging #31549 (4ccf3a5) into master (3293ca4) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #31549   +/-   ##
=======================================
  Coverage    81.3%    81.3%           
=======================================
  Files         733      733           
  Lines      204786   204801   +15     
=======================================
+ Hits       166517   166530   +13     
- Misses      38269    38271    +2

tao-stones · 2023-05-09T05:40:18Z

core/src/banking_stage.rs

@@ -390,7 +390,8 @@ impl BankingStage {
                        ),
                    };

-                let mut packet_receiver = PacketReceiver::new(id, packet_receiver);
+                let mut packet_receiver =
+                    PacketReceiver::new(id, packet_receiver, bank_forks.clone());


Passing bank_forks to PacketReceiver then to PacketDeserializer, so packets can be flagged with working_bank's feature set.

tao-stones · 2023-05-09T05:41:53Z

core/src/immutable_deserialized_packet.rs

@@ -55,7 +55,7 @@ impl ImmutableDeserializedPacket {

        // drop transaction if prioritization fails.
        let mut priority_details = sanitized_transaction
-            .get_transaction_priority_details()
+            .get_transaction_priority_details(packet.meta().round_compute_unit_price())


the particular feature flag is then read from packet's meta data, avoiding passing it through call stacks

tao-stones · 2023-05-09T05:42:52Z

core/src/packet_deserializer.rs

+            let mut packet_clone = packet_batch[*packet_index].clone();
+            packet_clone
+                .meta_mut()
+                .set_round_compute_unit_price(round_compute_unit_price_enabled);


flag packet with root_bank feature set

AshwinSekar · 2023-05-09T17:15:57Z

core/src/packet_deserializer.rs

@@ -42,9 +52,15 @@ impl PacketDeserializer {
        capacity: usize,
    ) -> Result<ReceivePacketResults, RecvTimeoutError> {
        let (packet_count, packet_batches) = self.receive_until(recv_timeout, capacity)?;
+
+        // get current root bank to get feature gate status
+        let _root_bank = self.bank_forks.read().unwrap().root_bank();


I'm not familiar with this specific feature round_compute_unit_price_enabled, but should we be getting it from the root bank? It seems like we should be checking the feature gates of the bank we're currently building, or our parent?
If it's a feature gate whole cluster won't cross epoch on their root bank at the same time which would lead to different timing for this feature to be activated across the cluster.

Excellent point! So this PR is prerequisite for #31469, where the feature gate and its functions are implemented.

The context is: the feature gate is required (for #31469) between banking stage receives packet from sigverify, and before inserting it to priority-sorted queue (Transaction storage), because the prioritization (eg compute-unit-price) defined in compute-budget IX will be rounded down to nearest lamports when the feature is activated.

This PR is getting banking stage part of code ready for that.

Was thinking about picking one between root_bank over working_bank, sounds like working_bank is the better option for the purpose.

Okay makes sense. Since it only affects block packing seems like it wouldn't actually affect consensus? Basically it's okay if some members of the cluster are rounding down and some are not - so does it require a feature gate?

This PR only affects leader code; to be more precious, would only affect at what numerical value of priority the packets are sorted in queue. For example, if a transaction sets compute-unit-price to 1_000_900; Before feature is enabled, the transaction will be prioritized as 1_000_900, after feature activation, it would be prioritized as 1_000_000. But all this change are in leader's thread queue only.

Just to clarify, the PR does not change code behavior, it only does the plumbing work. (it hardcoded the flag as false as feature is not activated, which is current behavior. )

It is a prerequisite of #31469 , where feature is actually added, and its status will affect how transactions are charged with priority fee, hence could affect consensus.

tao-stones · 2023-05-10T17:18:31Z

Checking the perf impact by read working_bank every receiving, and flag every received packet with feature status. Don't expect too much impact, runing banking-benching:
./cargo nightly run --release --manifest-path banking-bench/Cargo.toml

This branch:

[total_sent: 1152000, base_tx_count: 55296, txs_processed: 1187712, txs_landed: 1132416, total_us: 22317942, tx_total_us: 21732693]
{'name': 'banking_bench_total', 'median': '51617.66'}
{'name': 'banking_bench_tx_total', 'median': '53007.70'}
{'name': 'banking_bench_success_tx_total', 'median': '50740.16'}

Head of Master:

[total_sent: 1152000, base_tx_count: 55296, txs_processed: 1183488, txs_landed: 1128192, total_us: 21940348, tx_total_us: 21354720]
{'name': 'banking_bench_total', 'median': '52506.00'}
{'name': 'banking_bench_tx_total', 'median': '53945.92'}
{'name': 'banking_bench_success_tx_total', 'median': '51420.88'}

2. add packet meta flag; 3. prioritization_fee_cache read flag from its working bank directly; 4. immutable_deserialized_packet read flag from packet.meta;

2. packet_deserializer uses current root_bank from bank_fork to get feature status, use it to flag packet meta.

AshwinSekar

lgtm

apfitzge

I realize this was already merged, but I'm following-up to push back on it a bit. I think we should always be hesistant to add feature usage to things that do not affect consensus - I think it adds complexity that is not necessary, edge cases etc.

Leader's are already free to do whatever they want in terms of buffer prioritization - it's not enforcable by consensus. As an alternative, why not just start rounding down always for buffer prioritization, and we calculate actual fee based using the bank the tx is committed to.

To the best of my knowledge, there's no plans to backport this. So we have to wait for v1.16+ to activate anyway?

Additionally, the current method is already going to lead to some oddities around the feature activation:

gantt
    title Receiving Window Race
    dateFormat HH:mm
    axisFormat %H:%M

    section Banks
    Bank 1 (Not Leader): b1, 00:00, 40m
    Epoch Boundary: milestone, after b1
    Bank 2 (Leader): after b1, 40m

    section Receiving
    Receiving 1: 00:05, 5m
    Receiving 2: 00:35, 3m
    Receiving 3: 00:42, 3m

Packets from receive windows 1/2 will be unrounded, while packets from receive 3 will be rounded.

tao-stones · 2023-05-16T22:22:33Z

Valid point, we chatted offline, agreed that if leader started to round down before feature gate (effectively passing hardcoded true at runtime/src/transaction_priority_details.rs Ln28), user currently sets compute-unit-price lower than 1 lamport will be charged for a priority fee yet without being prioritized (since it'd round down to zero). That'd break current behavior, needs extra communication.

I think it is better to round down at both leader and fee calculation by same feature gate to be consistent in behavior, and focus the communication on this change is about incentivizing accurate compute-unit-limit economically.

tao-stones marked this pull request as ready for review May 9, 2023 05:38

tao-stones commented May 9, 2023

View reviewed changes

tao-stones requested review from apfitzge and AshwinSekar May 9, 2023 05:44

AshwinSekar reviewed May 9, 2023

View reviewed changes

tao-stones mentioned this pull request May 10, 2023

round down compute-unit-price to its nearest 1_000 microlamport #31469

Closed

tao-stones added 5 commits May 10, 2023 17:35

1. add lag to get_transaction_priority_details trait as parameter

0e32503

2. add packet meta flag; 3. prioritization_fee_cache read flag from its working bank directly; 4. immutable_deserialized_packet read flag from packet.meta;

1. inject bank_fork to packet_deserializer,

f5ae802

2. packet_deserializer uses current root_bank from bank_fork to get feature status, use it to flag packet meta.

update tests

0a2d32f

use working_bank instead of root_bank

20c1eb8

update comments

4ccf3a5

tao-stones force-pushed the refactor-pass-feature-status-to-deserialized-packet-via-packet-meta branch from 94fefbb to 4ccf3a5 Compare May 10, 2023 17:35

AshwinSekar approved these changes May 10, 2023

View reviewed changes

tao-stones merged commit 49f44f5 into solana-labs:master May 11, 2023

tao-stones deleted the refactor-pass-feature-status-to-deserialized-packet-via-packet-meta branch May 11, 2023 14:32

tao-stones mentioned this pull request May 11, 2023

Refactor pass feature status to deserialized packet ctor #31543

Closed

apfitzge reviewed May 16, 2023

View reviewed changes

tao-stones mentioned this pull request Oct 1, 2024

remove cu price rounding anza-xyz/agave#3047

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor pass feature status to deserialized packet via packet meta #31549

Refactor pass feature status to deserialized packet via packet meta #31549

tao-stones commented May 9, 2023 •

edited

Loading

codecov bot commented May 9, 2023 •

edited

Loading

tao-stones May 9, 2023 •

edited

Loading

tao-stones May 9, 2023 •

edited

Loading

tao-stones May 9, 2023

AshwinSekar May 9, 2023

tao-stones May 9, 2023

tao-stones May 9, 2023

AshwinSekar May 10, 2023

tao-stones May 10, 2023

tao-stones commented May 10, 2023

AshwinSekar left a comment

apfitzge left a comment

tao-stones commented May 16, 2023

Refactor pass feature status to deserialized packet via packet meta #31549

Refactor pass feature status to deserialized packet via packet meta #31549

Conversation

tao-stones commented May 9, 2023 • edited Loading

Problem

Summary of Changes

codecov bot commented May 9, 2023 • edited Loading

Codecov Report

tao-stones May 9, 2023 • edited Loading

Choose a reason for hiding this comment

tao-stones May 9, 2023 • edited Loading

Choose a reason for hiding this comment

tao-stones May 9, 2023

Choose a reason for hiding this comment

AshwinSekar May 9, 2023

Choose a reason for hiding this comment

tao-stones May 9, 2023

Choose a reason for hiding this comment

tao-stones May 9, 2023

Choose a reason for hiding this comment

AshwinSekar May 10, 2023

Choose a reason for hiding this comment

tao-stones May 10, 2023

Choose a reason for hiding this comment

tao-stones commented May 10, 2023

AshwinSekar left a comment

Choose a reason for hiding this comment

apfitzge left a comment

Choose a reason for hiding this comment

tao-stones commented May 16, 2023

tao-stones commented May 9, 2023 •

edited

Loading

codecov bot commented May 9, 2023 •

edited

Loading

tao-stones May 9, 2023 •

edited

Loading

tao-stones May 9, 2023 •

edited

Loading