Extend `upd_aggregate` and `add/del_publisher` to support 64 price components on pythnet #384

drozdziak1 · 2023-10-06T15:59:38Z

This change extends all methods that touch the price component array to use 64 publishers on Pythnet. This includes publisher addition/deletion and, notably, aggregation logic. The latter is the reason why we don't use all 128 publisher slots on Pythnet - the compiled size of upd_aggregate under 128 publishing slots breached the stack size limits, resulting in various tests failing with entered unreachable code panics.

Summary of changes

program/c/makefile - CFLAGS passthrough
program/c/src/oracle/oracle.h - Use a PC_PYTHNET feature macro to decide the value of PC_COMP_SIZE, disambiguate into PC_COMP_SIZE_SOLANA and PC_COMP_SIZE_PYTHNET.
program/rust/build.rs - detect the pythnet feature and define `PC_PYTHNET`` for the make build if set.
program/rust/src/accounts/price.rs - use a single big array for price components on Pythnet.
program/rust/src/tests/test_sizes.rs - Adjust size constants.

Testing

All unit tests and C static assertions continue to pass, including test_add_publisher which saturates the component array.

Review Highlights

oracle.h - It's not crystal clear to me how to prevent misuse between PC_COMP_SIZE, PC_COMP_SIZE_SOLANA and PC_COMP_SIZE_PYTHNET. PC_COMP_SIZE is the used size of the array, while the other two are total space available. In case of Solana, the two coincide

guibescos · 2023-10-09T10:10:41Z

program/c/src/oracle/oracle.h

+// Replace Solana component size's contribution with Pythnet's.
+//
+// Note: Make sure to account here for all PC_COMP_SIZE-sized arrays in the struct.
+#define PC_EXPECTED_CMD_UPD_TEST_T_SIZE_PYTHNET (560 \


Since this instruction is no longer used, I'd just get rid of the test altogether

guibescos · 2023-10-09T10:15:53Z

program/rust/build.rs

@@ -9,30 +9,50 @@ use {
 fn main() {
    let target_arch = std::env::var("CARGO_CFG_TARGET_ARCH").unwrap();

+    let has_feat_pythnet = std::env::var("CARGO_FEATURE_PYTHNET").is_ok();
+
    // OUT_DIR is the path cargo provides to a build directory under `target/` specifically for
    // isolated build artifacts. We use this to build the C program and then link against the


I think in this file you need to update the bindings codegen as well

guibescos · 2023-10-09T10:17:47Z

program/rust/src/accounts/price.rs

@@ -41,7 +38,7 @@ mod price_pythnet {
    };

    /// Pythnet-only extended price account format. This is extension
-    /// is an append-only change that adds extra publisher slots and
+    /// is an append-only change that extra publisher slots and


This comment change is grammatically incorrect

guibescos

Main comments :

Fix CI
I think the Pythnet rust oracle has the wrong PC_COMP_SIZE because you didn't update the bindgen code

drozdziak1 · 2023-10-09T13:59:06Z

Summary of changes (since last review pass)

Build.rs bindgen

program/rust/build.rs - Provide -DPC_PYTHNET=1 to bindgen as well when building with pythnet.

Sanity Checks

program/rust/src/tests/test_size.rs - Explicitly verify that generated bindings point at the expected PC_COMP_SIZE value

CI

Why was it failing? - The make build non-zero exit was ignored in build.rs before this change. The ignored failure was a make call unable to find Solana build files. At the same time, this part of the build.rs was non-essential to cargo clippy which does not do linking, meaning that the silent failure would still let formatting CI pass.
How was this solved? - We add a new feature named check intended for use with cargo-check and cargo-clippy that ommits the make build explicitly. This solution assumes that the build will never get to a linking stage.A corresponding --features check flag is added to the pre-commit config's cargo-clippy check.

Fortify `pre-commit`'s `cargo clippy` stage

As per official Cargo docs, cargo-clippy will not reach code behind the pythnet feature on its own. A distinct pre-commit job is added to handle that case as well.

drozdziak1 · 2023-10-09T14:56:35Z

Remaining items

- Remove CMD_UPD_TEST_T_[...]
- Add a non-trivial test to saturate component array and verify that

guibescos · 2023-10-09T15:16:30Z

program/rust/src/tests/test_sizes.rs

@@ -58,6 +59,9 @@ fn test_sizes() {
            c_oracle_header::PC_COMP_SIZE_PYTHNET,
        };

+        // Sanity-check the Pythnet PC_COMP_SIZE
+        assert_eq!(PC_COMP_SIZE, 64);


guibescos

I think it's in a good state to merge. I'm still a bit scared we accidentally link the Solana smart contract to the Pythnet C library

jayantk · 2023-10-06T17:38:06Z

program/c/src/oracle/oracle.h

-#define PC_COMP_SIZE         32
-#define PC_COMP_SIZE_V2     128
+#define PC_COMP_SIZE_SOLANA      32
+#define PC_COMP_SIZE_PYTHNET     128


suggest calling these PC_NUM_COMP_SOLANA to distinguish values which are numbers from those which are byte size lengths

jayantk · 2023-10-06T17:39:09Z

program/rust/src/accounts/price.rs

@@ -41,7 +38,7 @@ mod price_pythnet {
    };

    /// Pythnet-only extended price account format. This is extension
-    /// is an append-only change that adds extra publisher slots and
+    /// is an append-only change that extra publisher slots and


not sure why you deleted the word "adds" here?

jayantk · 2023-10-10T12:54:04Z

agree with @guibescos about accidentally linking mismatched binaries. There's probably an easy way to test that -- actually if we have a unit test that uses > 32 publishers in the simulator, then I think that will catch it. The simulator loads the built bpf artifact, so that should account for the linking too.

drozdziak1 · 2023-10-19T12:31:33Z

Changes since last review

Changed COMP_SIZE constants to NUM_COMP
Addressed comments about linking the Rust program against the wrong C binary.

Failure modes for pythnet/solana C/Rust mismatch

The oracle.h header fails to use the right PC_PYTHNET value when generating bindings. Here's how we detect this:
** We verify value of PC_NUM_COMP in a unit test
The make build could make a bad caching choice and assume that a Solana *.o is fine to link under a Pythnet build. Here's how we prevent this:
** The C program exposes c_upd_aggregate_solana or c_upd_aggregate_pythnetdepending on #ifdef PC_PYTHNET
** PC_PYTHNET is defined in a dynamically populated header (features.h) that must exist at build time (C preprocessor would get mad otherwise). This choice to use a header was made to work around difficulties passing -D C flags to the Solana makefile that gets included.
** features.h is always regenerated on subsequent make builds - done by adding it to the .PHONY target. other targets depend on features.h being built, forcing it to be regenerated.
** After make build, the Rust linking stage has a clear expectation of c_upd_aggregate_<network> existing, meaning that C and Rust must agree on it being the pythnet or solana name, otherwise causing a linking error.

jayantk

nice! this solution makes sense to me. thanks for digging into all the linking stuff.

drozdziak1 · 2023-10-19T13:49:19Z

Thank you! This will be followed up by an explicit 32/64 publisher test in a separate change.

program: Add a #define-based C feature for choice of PC_COMP_SIZE

590c521

guibescos reviewed Oct 9, 2023

View reviewed changes

drozdziak1 added 5 commits October 9, 2023 13:51

program/rust: fix price.rs typo

88d236a

Fix bindgen build for pythnet, test_sizes: add PC_COMP_SIZE asserts

4001805

program/rust: New check feature skips make-build in formatting CI

d2c4ac8

fix build.rs formatting

b6d30f4

.pre-commit-config.yaml: Also check under pythnet feature

21c97d5

guibescos reviewed Oct 9, 2023

View reviewed changes

guibescos previously approved these changes Oct 9, 2023

View reviewed changes

jayantk previously approved these changes Oct 10, 2023

View reviewed changes

oracle.h: Remove cmd_upd_test_t

7e1929c

drozdziak1 dismissed stale reviews from jayantk and guibescos via 7e1929c October 10, 2023 13:34

drozdziak1 added 6 commits October 17, 2023 15:14

build.rs: Print OUT_DIR, PC_PYTHNET make variable instead of -D flag

46f05ab

program/c: dynamic header for features, dynamic c_upd_aggregate name

bc9c3f0

program: rename COMP_SIZE to NUM_COMP in C and Rust

d8f20db

makefile, build.rs: Clarify comments

56b88a1

upd_price.rs: remove stray log statement

0a24a8a

upd_price.rs: missed another one

4052ff6

jayantk approved these changes Oct 19, 2023

View reviewed changes

drozdziak1 merged commit 63e6356 into main Oct 19, 2023

drozdziak1 deleted the drozdziak1/aggregation-logic-64-pubs branch October 19, 2023 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Extend `upd_aggregate` and `add/del_publisher` to support 64 price components on pythnet #384

Extend `upd_aggregate` and `add/del_publisher` to support 64 price components on pythnet #384

Uh oh!

drozdziak1 commented Oct 6, 2023

Uh oh!

guibescos Oct 9, 2023

Uh oh!

guibescos Oct 9, 2023

Uh oh!

guibescos Oct 9, 2023

Uh oh!

guibescos left a comment

Uh oh!

drozdziak1 commented Oct 9, 2023

Uh oh!

drozdziak1 commented Oct 9, 2023 •

edited

Loading

Uh oh!

guibescos Oct 9, 2023

Uh oh!

guibescos left a comment

Uh oh!

jayantk Oct 6, 2023

Uh oh!

jayantk Oct 6, 2023

Uh oh!

jayantk commented Oct 10, 2023

Uh oh!

drozdziak1 commented Oct 19, 2023

Uh oh!

jayantk left a comment

Uh oh!

drozdziak1 commented Oct 19, 2023

Uh oh!

Uh oh!

Extend upd_aggregate and add/del_publisher to support 64 price components on pythnet #384

Extend upd_aggregate and add/del_publisher to support 64 price components on pythnet #384

Uh oh!

Conversation

drozdziak1 commented Oct 6, 2023

Summary of changes

Testing

Review Highlights

Uh oh!

guibescos Oct 9, 2023

Choose a reason for hiding this comment

Uh oh!

guibescos Oct 9, 2023

Choose a reason for hiding this comment

Uh oh!

guibescos Oct 9, 2023

Choose a reason for hiding this comment

Uh oh!

guibescos left a comment

Choose a reason for hiding this comment

Uh oh!

drozdziak1 commented Oct 9, 2023

Summary of changes (since last review pass)

Build.rs bindgen

Sanity Checks

CI

Fortify pre-commit's cargo clippy stage

Uh oh!

drozdziak1 commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Remaining items

Uh oh!

guibescos Oct 9, 2023

Choose a reason for hiding this comment

Uh oh!

guibescos left a comment

Choose a reason for hiding this comment

Uh oh!

jayantk Oct 6, 2023

Choose a reason for hiding this comment

Uh oh!

jayantk Oct 6, 2023

Choose a reason for hiding this comment

Uh oh!

jayantk commented Oct 10, 2023

Uh oh!

drozdziak1 commented Oct 19, 2023

Changes since last review

Failure modes for pythnet/solana C/Rust mismatch

Uh oh!

jayantk left a comment

Choose a reason for hiding this comment

Uh oh!

drozdziak1 commented Oct 19, 2023

Uh oh!

Uh oh!

Extend `upd_aggregate` and `add/del_publisher` to support 64 price components on pythnet #384

Extend `upd_aggregate` and `add/del_publisher` to support 64 price components on pythnet #384

Fortify `pre-commit`'s `cargo clippy` stage

drozdziak1 commented Oct 9, 2023 •

edited

Loading