Drozdziak1/p2w attest cont mapping reload #330

drozdziak1 · 2022-10-06T13:27:42Z

This changeset implements continuous reloads of the mapping account, without interruptions. The complexity bump is there but still appears worth the hassle.

This change takes care of recoverable mapping crawling errors (e.g. malformed single price on single product is no longer dropping otherwise good different prices and products in the mapping in favor of a warn message)

…-cont-mapping-reload

ali-behjati · 2022-10-19T10:54:16Z

solana/pyth2wormhole/client/src/main.rs

+        );

-    let results = futures::future::join_all(attestation_sched_futs).await; // May never finish for daemon mode
+        let mut batches: Vec<_> = attestation_cfg


I think we can compute these only when needed (when config has changed below). Performance-wise it's not much different but it will result in more clear logs.

ali-behjati · 2022-10-19T11:05:08Z

I think the code is technically correct and I don't see any issue (just why tilt is complaining?).

My concern is mostly the complexity of this code, it is very complex now.

Now that we are not using constraints to update prices (and we are doing it as fast as possible) we can let go of the constraint check totally, and then have it as two loop. One loop is always attesting based on the groups. One loop is crawling mapping accounts and updating the groups if needed. This will be reusing everything here and removing many existing code to make it simpler.

I think we can merge this one and go for simplifying it like ^ in another PR.

Let's hear what other folks think and if they agree I'll approve this PR.

ali-behjati

I talked with Stan about this PR and here is a summary:

The current code can be further simplified if we can move out the non-deamon logic and it will be much easier to reason about.
Also moving some logic in the long functions to separate functions will improve the readability of the code
There is no way forward to separate the loops with current design. To do it we should step back and change the whole structure of code. Given that it will be a huge refactor it's not clear for me that the outcome is worth the effort.

jayantk

I think the cryptographic hash thing should be fixed now.

Aside from that, I think @ali-bahjati 's comment above is right. Let's merge this, but would be good to follow up with a refactoring PR to make the logic easier to understand.

jayantk · 2022-10-06T13:51:37Z

solana/pyth2wormhole/client/src/attestation_cfg.rs


-    /// Trigger attestation if price changes by the specified percentage.
+    /// Trigger attestation if price changes by the specified
+    /// percentage, expressed in integer parts per thousand.


i suggest using basis points as the units here (1 basis point = 1/100th of a %), which is a very standard financial measurement

jayantk · 2022-10-19T13:48:33Z

solana/pyth2wormhole/client/src/main.rs

-        .collect();
+        info!("Spinning up attestation sched jobs");
+        if daemon {
+            let mut hasher = DefaultHasher::new();


I think you want a cryptographic hash here -- if you get a hash collision, the code below isn't going to do the right thing.

solana/pyth2wormhole/client/src/main.rs

jayantk · 2022-10-19T13:51:56Z

solana/pyth2wormhole/client/src/attestation_cfg.rs

+    /// percentage, expressed in integer parts per thousand.
    #[serde(default)]
-    pub price_changed_pct: Option<f64>,
+    pub price_changed_ppt: Option<u64>,


i really really think you should use basis points here. 1 / 1000th is a weird unit for prices, but 1 / 10000 (1 bp) is very standard. This configuration option is going to confuse people.

jayantk · 2022-10-19T13:55:41Z

solana/pyth2wormhole/client/src/main.rs

-            message_q_mtx.clone(),
-        )
-    });
+    // This loop governs mapping account reloads in daemon mode. It is


this function is long and the logic is hard to understand because it supports all these different options around daemon / no daemon.

I think it would be good to refactor this by extracting out helper functions and then possibly having two separate top-level functions for daemon / non-daemon mode. Each of those functions could use the helpers to avoid code duplication.

...lity and remove most warnings

…-cont-mapping-reload

jayantk

nice! I left you a couple minor comments. I suggest addressing them in a fast follow up PR so that it's easier to review.

jayantk · 2022-10-20T13:40:06Z

solana/pyth2wormhole/client/src/attestation_cfg.rs

+    /// percentage, expressed in integer base points (1bp = 0.01%)
    #[serde(default)]
-    pub price_changed_pct: Option<f64>,
+    pub price_changed_bp: Option<u64>,


nitpick: usually you abbreviate basis points to "bps"

jayantk · 2022-10-20T13:42:41Z

solana/pyth2wormhole/client/src/main.rs

+        );
+
+        // Hash currently known config
+        let mut hasher = DefaultHasher::new();


i think i left a comment last time about using a cryptographic hash here. The code below depends on no collisions for correctness.

That makes sense. I think the std HashMap has internal collision detection that isn't part of the hasher. I assumed it would be enough, but a nicer 32-byte scheme will fit better here.

jayantk · 2022-10-20T13:44:02Z

solana/pyth2wormhole/client/src/main.rs

+        debug!(
+            "Attestation config (includes mapping accounts):\n{:#?}",
+            attestation_cfg
+        );


this debug statement needs to be in the if i think

also the code in the if statement above is duplicated across this method and the one below. consider extracting out a helper function.

I left it here to print the config regardless, it's the "includes mapping accounts" part that's new.

jayantk · 2022-10-20T13:46:14Z

solana/pyth2wormhole/client/src/main.rs

+    let message_q_mtx = Arc::new(Mutex::new(P2WMessageQueue::new(
+        Duration::from_millis(attestation_cfg.min_msg_reuse_interval_ms),
+        attestation_cfg.max_msg_accounts as usize,
+    )));


I think the state of this queue gets reset each time the attestation batches change. Does that mean we can end up reusing accounts more frequently than intended each time the mapping account reloads (and the batches get recreated)?

this is not a big deal given that the mapping accounts reload very infrequently. maybe leave a // todo comment if this is the case.

This was an easy fix, just moved the mutex constructor to before the loop so that it and its state is reused. Async runtimes must properly deallocate variables in scope of an aborted task (to avoid counter-intuitive mem leaks), so it should work well now and in the future

Stan Drozd added 4 commits October 5, 2022 14:26

pyth2wormhole-client: automatically crawl mapping based on interval

2da0781

Make the mapping crawl automation seamless

00ddd4f

pyth2wormhole-client: Make mapping crawling routine more robust

c8ba3d9

This change takes care of recoverable mapping crawling errors (e.g. malformed single price on single product is no longer dropping otherwise good different prices and products in the mapping in favor of a warn message)

pyth2wormhole-client: Move mapping crawl sleep near logic it affects

28f9634

drozdziak1 requested review from Reisen, ali-behjati, daviesalex and tompntn October 6, 2022 13:27

Stan Drozd added 4 commits October 6, 2022 15:34

pyth2wormhole-client: remove stray comment

404cb08

Merge remote-tracking branch 'origin/main' into drozdziak1/p2w-attest…

a78ef32

…-cont-mapping-reload

Merge remote-tracking branch 'origin/main' into drozdziak1/p2w-attest…

82e6d0a

…-cont-mapping-reload

Merge remote-tracking branch 'origin/main' into drozdziak1/p2w-attest…

8b00b15

…-cont-mapping-reload

ali-behjati reviewed Oct 19, 2022

View reviewed changes

ali-behjati requested a review from jayantk October 19, 2022 10:58

Stan Drozd added 2 commits October 19, 2022 14:27

pyth2wormhole: Fix faulty merge with master

290e612

pyth2wormhole-client: Fix mapping crawl price counting

b5e5ccf

ali-behjati reviewed Oct 19, 2022

View reviewed changes

jayantk reviewed Oct 19, 2022

View reviewed changes

Stan Drozd added 4 commits October 20, 2022 14:53

pyth2wormhole-client: split daemon/non-daemon, improve readabi[...]

41a0c39

...lity and remove most warnings

Merge remote-tracking branch 'origin/main' into drozdziak1/p2w-attest…

a4f81e4

…-cont-mapping-reload

pyth2wormhole-client: parts-per-thousand -> base points

95f17cb

pyth2wormhole-client: inaccurate comment

929463f

jayantk approved these changes Oct 20, 2022

View reviewed changes

Stan Drozd added 2 commits October 24, 2022 16:08

p2w-client: review advice - bp -> bps, std hasher -> sha256

9c0a199

pyth2wormhole-client: reuse message queue across mapping lookups

691e213

drozdziak1 merged commit 3eb2bea into main Oct 24, 2022

drozdziak1 deleted the drozdziak1/p2w-attest-cont-mapping-reload branch October 24, 2022 15:19

Drozdziak1/p2w attest cont mapping reload #330

Drozdziak1/p2w attest cont mapping reload #330

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ali-behjati commented Oct 19, 2022

Uh oh!

ali-behjati left a comment

Choose a reason for hiding this comment

Uh oh!

jayantk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayantk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

drozdziak1 commented Oct 6, 2022 •

edited

Loading