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

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

* program/rust: fix price.rs typo

* Fix bindgen build for pythnet, test_sizes: add PC_COMP_SIZE asserts

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

* fix build.rs formatting

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

* oracle.h: Remove cmd_upd_test_t

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

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

* program: rename COMP_SIZE to NUM_COMP in C and Rust

* makefile, build.rs: Clarify comments

* upd_price.rs: remove stray log statement

* upd_price.rs: missed another one

Author: drozdziak1

.pre-commit-config.yaml

@@ -14,8 +14,13 @@ repos:
       language: "rust"
       entry: cargo +nightly-2023-03-01 fmt
       pass_filenames: false
-    - id: cargo-clippy
-      name: Cargo clippy
+    - id: cargo-clippy-solana
+      name: Cargo Clippy Solana
       language: "rust"
-      entry : cargo +nightly-2023-03-01 clippy --tests -- -D warnings
+      entry : cargo +nightly-2023-03-01 clippy --tests --features check -- -D warnings
+      pass_filenames : false
+    - id: cargo-clippy-pythnet
+      name: Cargo Clippy Pythnet
+      language: "rust"
+      entry : cargo +nightly-2023-03-01 clippy --tests --features pythnet,check -- -D warnings
       pass_filenames : false

pcapps/pyth_csv.cpp

@@ -83,7 +83,7 @@ void csv_print::print_header()
             << ",prev_slot"
             << ",prev_price"
             << ",prev_conf";
-  for(unsigned i=0; i != PC_COMP_SIZE; ++i ) {
+  for(unsigned i=0; i != PC_NUM_COMP; ++i ) {
     std::cout << ",comp_status_" << i
               << ",comp_price_" << i
               << ",comp_conf_" << i
@@ -156,7 +156,7 @@ void csv_print::parse_price( replay& rep )
               << ',' << cptr->agg_.conf_
               << ',' << cptr->agg_.pub_slot_;
   }
-  for( unsigned i=ptr->num_; i != PC_COMP_SIZE; ++i ) {
+  for( unsigned i=ptr->num_; i != PC_NUM_COMP; ++i ) {
     std::cout << ",,,,";
   }
   std::cout << std::endl;

program/c/makefile

@@ -11,11 +11,25 @@ else
   include $(SOLANA)/sdk/sbf/c/sbf.mk
 endif
 
+# Propagate the PC_PYTHNET feature by conditionally defining it in a
+# features.h header. The makefile included from Solana SDK does not
+# have an easy way to pass extra C flags which motivates this approach.
+ifdef PC_PYTHNET
+  FEATURES_H_BODY:="\#pragma once\n\#define PC_PYTHNET 1"
+else
+  FEATURES_H_BODY:="\#pragma once"
+endif
+
+
+.PHONY: features.h # Putting this in .PHONY makes sure the header is always regenerated
+features.h:
+	echo $(FEATURES_H_BODY) > src/oracle/features.h
+
 
 # Bundle C code compiled to bpf for use by rust
 # The all target is defined by the solana makefile included above and generates the needed .o file.
 .PHONY: cpyth-bpf
-cpyth-bpf: all
+cpyth-bpf: features.h all
 	bash -c "ar rc $(OUT_DIR)/libcpyth-bpf.a $(OUT_DIR)/oracle/*.o"
 
 
@@ -24,14 +38,14 @@ cpyth-bpf: all
 # 1) Compile C code to system architecture for use by rust's cargo test
 # 2) Bundle C code compiled to system architecture for use by rust's cargo test
 .PHONY: cpyth-native
-cpyth-native:
+cpyth-native: features.h
 	gcc -c ./src/oracle/native/upd_aggregate.c -o $(OUT_DIR)/cpyth-native.o -fPIC
 	ar rcs $(OUT_DIR)/libcpyth-native.a $(OUT_DIR)/cpyth-native.o
 
 
 # Note: there's probably a smart way to do this with wildcards but I (jayant) can't figure it out
 .PHONY: test
-test:
+test: features.h
 	mkdir -p $(OUT_DIR)/test/
 	gcc -c ./src/oracle/model/test_price_model.c -o $(OUT_DIR)/test/test_price_model.o -fPIC
 	gcc -c ./src/oracle/sort/test_sort_stable.c -o $(OUT_DIR)/test/test_sort_stable.o -fPIC

program/c/src/oracle/native/upd_aggregate.c

@@ -7,8 +7,13 @@ char heap_start[8192];
 #define static_assert _Static_assert
 
 #include "../upd_aggregate.h"
+#include "features.h"
 
-extern bool c_upd_aggregate( pc_price_t *ptr, uint64_t slot, int64_t timestamp ){
+#ifdef PC_PYTHNET
+extern bool c_upd_aggregate_pythnet( pc_price_t *ptr, uint64_t slot, int64_t timestamp ){
+#else
+extern bool c_upd_aggregate_solana( pc_price_t *ptr, uint64_t slot, int64_t timestamp ){
+#endif
   return upd_aggregate(ptr, slot, timestamp );
 }
 

program/c/src/oracle/oracle.h

@@ -2,6 +2,7 @@
 
 #include <stdbool.h>
 #include "util/compat_stdint.h"
+#include "features.h"
 
 #ifdef __cplusplus
 extern "C" {
@@ -20,8 +21,18 @@ extern "C" {
 #define PC_PUBKEY_SIZE       32
 #define PC_PUBKEY_SIZE_64   (PC_PUBKEY_SIZE/sizeof(uint64_t))
 #define PC_MAP_TABLE_SIZE   640
-#define PC_COMP_SIZE         32
-#define PC_COMP_SIZE_V2     128
+
+  // Total price component slots available
+#define PC_NUM_COMP_SOLANA      32
+#define PC_NUM_COMP_PYTHNET     128
+
+// PC_NUM_COMP - number of price components in use
+#ifdef PC_PYTHNET
+// Not whole PC_NUM_COMP_PYTHNET because of stack issues appearing in upd_aggregate()
+#define PC_NUM_COMP 64
+#else
+#define PC_NUM_COMP PC_NUM_COMP_SOLANA
+#endif
 
 #define PC_PROD_ACC_SIZE    512
 #define PC_EXP_DECAY         -9
@@ -194,10 +205,23 @@ typedef struct pc_price
   uint64_t        prev_conf_;         // confidence interval of previous aggregate with TRADING status
   int64_t         prev_timestamp_;    // unix timestamp of previous aggregate with TRADING status
   pc_price_info_t agg_;               // aggregate price information
-  pc_price_comp_t comp_[PC_COMP_SIZE];// component prices
+  pc_price_comp_t comp_[PC_NUM_COMP];// component prices
 } pc_price_t;
 
+#ifdef PC_PYTHNET
+
+// Replace Solana component size's contribution with Pythnet's
+#define PC_EXPECTED_PRICE_T_SIZE_PYTHNET (3312 \
+					- PC_NUM_COMP_SOLANA * sizeof(pc_price_comp_t) \
+					+ PC_NUM_COMP * sizeof(pc_price_comp_t) \
+					)
+
+static_assert( sizeof( pc_price_t ) == PC_EXPECTED_PRICE_T_SIZE_PYTHNET, "" );
+#undef PC_EXPECTED_PRICE_SIZE_PYTHNET
+
+#else
 static_assert( sizeof( pc_price_t ) == 3312, "" );
+#endif
 
 // This constant needs to be an upper bound of the price account size, it is used within pythd for ztsd.
 // It is set tighly to the current price account + 96 component prices + 48 bytes for cumulative sums
@@ -383,19 +407,6 @@ typedef struct cmd_upd_price
 
 static_assert( sizeof( cmd_upd_price_t ) == 40, "" );
 
-typedef struct cmd_upd_test
-{
-  uint32_t     ver_;
-  int32_t      cmd_;
-  uint32_t     num_;
-  int32_t      expo_;
-  int8_t       slot_diff_[PC_COMP_SIZE];
-  int64_t      price_[PC_COMP_SIZE];
-  uint64_t     conf_[PC_COMP_SIZE];
-} cmd_upd_test_t;
-
-static_assert( sizeof( cmd_upd_test_t ) == 560, "" );
-
 // structure of clock sysvar account
 typedef struct sysvar_clock
 {

program/c/src/oracle/upd_aggregate.c

@@ -4,8 +4,14 @@
 #include <solana_sdk.h>
 #include "oracle.h"
 #include "upd_aggregate.h"
+#include "features.h"
 
-extern bool c_upd_aggregate( pc_price_t *ptr, uint64_t slot, int64_t timestamp ){
+// Dynamically deciding the name prevents linking the wrong C binary flavor
+#ifdef PC_PYTHNET
+extern bool c_upd_aggregate_pythnet( pc_price_t *ptr, uint64_t slot, int64_t timestamp ){
+#else
+extern bool c_upd_aggregate_solana( pc_price_t *ptr, uint64_t slot, int64_t timestamp ){
+#endif
   return upd_aggregate(ptr, slot, timestamp );
 }
 

program/c/src/oracle/upd_aggregate.h

@@ -5,17 +5,16 @@
 #include "model/price_model.c" /* FIXME: HACK TO DEAL WITH DOCKER LINKAGE ISSUES */
 #include "pd.h"
 
-
 #ifdef __cplusplus
 extern "C" {
 #endif
 
 typedef struct pc_qset
 {
-  pd_t      iprice_[PC_COMP_SIZE];
-  pd_t      uprice_[PC_COMP_SIZE];
-  pd_t      lprice_[PC_COMP_SIZE];
-  pd_t      weight_[PC_COMP_SIZE];
+  pd_t      iprice_[PC_NUM_COMP];
+  pd_t      uprice_[PC_NUM_COMP];
+  pd_t      lprice_[PC_NUM_COMP];
+  pd_t      weight_[PC_NUM_COMP];
   int64_t   decay_[1+PC_MAX_SEND_LATENCY];
   int64_t   fact_[PC_FACTOR_SIZE];
 } pc_qset_t;
@@ -163,7 +162,7 @@ static inline bool upd_aggregate( pc_price_t *ptr, uint64_t slot, int64_t timest
   {
     uint32_t numv  = 0;
     uint32_t nprcs = (uint32_t)0;
-    int64_t  prcs[ PC_COMP_SIZE * 3 ]; // ~0.75KiB for current PC_COMP_SIZE (FIXME: DOUBLE CHECK THIS FITS INTO STACK FRAME LIMIT)
+    int64_t  prcs[ PC_NUM_COMP * 3 ]; // ~0.75KiB for current PC_NUM_COMP (FIXME: DOUBLE CHECK THIS FITS INTO STACK FRAME LIMIT)
     for ( uint32_t i = 0; i != ptr->num_; ++i ) {
       pc_price_comp_t *iptr = &ptr->comp_[i];
       // copy contributing price to aggregate snapshot
@@ -195,7 +194,7 @@ static inline bool upd_aggregate( pc_price_t *ptr, uint64_t slot, int64_t timest
     // note: numv>0 and nprcs = 3*numv at this point
     int64_t agg_p25;
     int64_t agg_p75;
-    int64_t scratch[ PC_COMP_SIZE * 3 ]; // ~0.75KiB for current PC_COMP_SIZE (FIXME: DOUBLE CHECK THIS FITS INTO STACK FRAME LIMIT)
+    int64_t scratch[ PC_NUM_COMP * 3 ]; // ~0.75KiB for current PC_NUM_COMP (FIXME: DOUBLE CHECK THIS FITS INTO STACK FRAME LIMIT)
     price_model_core( (uint64_t)nprcs, prcs, &agg_p25, &agg_price, &agg_p75, scratch );
 
     // get the left and right confidences

program/rust/Cargo.toml

@@ -43,6 +43,7 @@ lazy_static = "1.4.0"
 # to cargo-build-bpf at that point - we manually capture them at
 # compile-time and pass on to the child process.
 [features]
+check = [] # Skips make build in build.rs, use with cargo-clippy and cargo-check
 debug = []
 library = []
 pythnet = [] # logic-affecting features start with this one

program/rust/build.rs

@@ -1,47 +1,68 @@
 use {
     bindgen::Builder,
     std::{
-        path::PathBuf,
+        path::{
+            Path,
+            PathBuf,
+        },
         process::Command,
     },
 };
 
 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();
+    let has_feat_check = std::env::var("CARGO_FEATURE_CHECK").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
     // resulting static library. This allows building the program when used as a dependency of
     // another crate.
     let out_dir = std::env::var("OUT_DIR").unwrap();
+
+    // Useful for C binary debugging, not printed without -vv cargo flag
+    eprintln!("OUT_DIR is {}", out_dir);
     let out_dir = PathBuf::from(out_dir);
 
+    let mut make_extra_flags = vec![];
+    let mut clang_extra_flags = vec![];
+
+    if has_feat_pythnet {
+        // Define PC_PYTHNET for the C binary build
+        make_extra_flags.push("PC_PYTHNET=1");
+
+        // Define PC_PYTHNET for the bindings build
+        clang_extra_flags.push("-DPC_PYTHNET=1");
+    }
+
     let mut make_targets = vec![];
     if target_arch == "bpf" {
         make_targets.push("cpyth-bpf");
     } else {
         make_targets.push("cpyth-native");
     }
-    make_targets.push("test");
 
-    // We must forward OUT_DIR as an env variable to the make script otherwise it will output
-    // its artifacts to the wrong place.
-    std::process::Command::new("make")
-        .env("VERBOSE", "1")
-        .env("OUT_DIR", out_dir.display().to_string())
-        .current_dir("../c")
-        .args(make_targets)
-        .status()
-        .expect("Failed to build C program");
+    make_targets.push("test");
 
-    // Link against the right library for the architecture
-    if target_arch == "bpf" {
-        println!("cargo:rustc-link-lib=static=cpyth-bpf");
+    // When the `check` feature is active, we skip the make
+    // build. This is used in pre-commit checks to avoid requiring
+    // Solana in its GitHub Action.
+    if has_feat_check {
+        eprintln!("WARNING: `check` feature active, make build is skipped");
     } else {
-        println!("cargo:rustc-link-lib=static=cpyth-native");
+        do_make_build(make_extra_flags, make_targets, &out_dir);
+
+        // Link against the right library for the architecture
+        if target_arch == "bpf" {
+            println!("cargo:rustc-link-lib=static=cpyth-bpf");
+        } else {
+            println!("cargo:rustc-link-lib=static=cpyth-native");
+        }
+
+        println!("cargo:rustc-link-lib=static=cpyth-test");
+        println!("cargo:rustc-link-search={}", out_dir.display());
     }
-    println!("cargo:rustc-link-lib=static=cpyth-test");
-    println!("cargo:rustc-link-search={}", out_dir.display());
 
     std::fs::create_dir("./codegen").unwrap_or_else(|e| {
         eprintln!(
@@ -53,16 +74,41 @@ fn main() {
     // Generate and write bindings
     let bindings = Builder::default()
         .clang_arg(format!("-I{:}", get_solana_inc_path().display()))
+        .clang_args(clang_extra_flags)
         .header("./src/bindings.h")
         .rustfmt_bindings(true)
         .generate()
         .expect("Unable to generate bindings");
+
     bindings
         .write_to_file("./codegen/bindings.rs")
         .expect("Couldn't write bindings!");
 
     // Rerun the build script if either the rust or C code changes
-    println!("cargo:rerun-if-changed=../")
+    println!("cargo:rerun-if-changed=../");
+}
+
+fn do_make_build(extra_flags: Vec<&str>, targets: Vec<&str>, out_dir: &Path) {
+    // We must forward OUT_DIR as an env variable to the make script otherwise it will output
+    // its artifacts to the wrong place.
+    let make_output = std::process::Command::new("make")
+        .env("VERBOSE", "1")
+        .env("OUT_DIR", out_dir.display().to_string())
+        .current_dir("../c")
+        .args(extra_flags)
+        .args(targets)
+        .output()
+        .expect("Failed to run make for C oracle program");
+
+    if !make_output.status.success() {
+        panic!(
+            "C oracle make build did not exit with 0 (code
+	({:?}).\n\nstdout:\n{}\n\nstderr:\n{}",
+            make_output.status.code(),
+            String::from_utf8(make_output.stdout).unwrap_or("<non-utf8>".to_owned()),
+            String::from_utf8(make_output.stderr).unwrap_or("<non-utf8>".to_owned())
+        );
+    }
 }
 
 /// Find the Solana C header bindgen