Prestwich/super pending #566

prestwich · 2021-11-08T17:46:37Z

Motivation

Provide a reasonable and working alternative to gas escalator middleware.

Solution

Introduce a new Future EscalatingPending that broadcasts a series of transactions
- functions similarly to PendingTransaction but with a simpler (but less informative) state machine
- it returns the first receipt to confirm.
- it also returns the first RPC error to occur
- it allows users to configure polling and broadcast intervals
Add a middleware method send_escalating
- allows users to instantiate with a TypedTransaction and an escalation policy
- may be better as a middleware extension trait?

things I don't like

Invariant enforcement is done in the Middleware method rather than the constructor.
- This is brittle
- it's hard to see how to sign and nonce correctly otherwise
Naming

PR Checklist

Added Tests
Added Documentation
Updated the changelog

luketchang · 2021-11-08T18:38:59Z

ethers-providers/src/pending_escalator.rs

+                    {
+                        // then if we have a TX to broadcast, start
+                        // broadcasting it
+                        if let Some(next_to_broadcast) = this.txns.pop() {


So if we have already broadcast the most escalated tx, we will just wait on that one indefinitely?

luketchang · 2021-11-08T18:49:44Z

ethers-providers/src/pending_escalator.rs

+                        completed!(this, Ok(receipt));
+                    }
+                    // rewake until drained
+                    Poll::Ready(Ok(None)) => cx.waker().wake_by_ref(),


Is this cx.waker().wake_by_ref() necessary? Won't executor automatically put the EscalatingPending future back in queue? Other paths here that return Poll::Pending don't call wake

the goal is to explicitly notify the executor that this is ready to be polled again immediately. i.e. override the executor's scheduling and jump to the front of the queue as much as we have the ability to, as we know that there's no async operation happening to wait on

this section looks like it could be wrapped in a loop over all futures? So that Poll::Ready(Ok(None)) => merely results in a continue?
Is the order in which they should be polled here important, if not then Vec::swap_remove could be useful.

I mean it kinda is right now, it's just ensuring that it allows the executor to schedule each step of the loop and interleave other work

Looping over all futures is an anti-pattern I think, the state machine may be slightly easier to write but as @prestwich says it's better to do it on a per-fut basis

Isn't the CheckingReceipts state essentially a FuturesOrdered but we only poll one future at a time?

gakonst

some minor qs, love the direction

ethers-providers/src/lib.rs

gakonst · 2021-11-08T20:11:10Z

ethers-providers/src/lib.rs

+                r
+            })
+            .map(|req| async move {
+                self.sign(req.rlp(chain_id), from).await.map(|sig| req.rlp_signed(chain_id, &sig))


Is this really bad UX? Should we change it to something better?

a shortcut function for signing transactions might be good?

prestwich · 2021-11-08T20:40:26Z

ethers-providers/src/pending_escalator.rs

+                broadcast_checks!(cx, this, fut);
+            }
+            Sleeping(instant) => {
+                if instant.elapsed() > *this.polling_interval {


@gakonst please check this pattern out, as I am unclear why we use Delay futures elsewhere. is there some significant behavior difference I'm missing?

the futures_timer Delay type uses an additional global timerhandle thread that makes sure the delayed task gets woken up at the right time, however, futures_timer hasn't been updated in a while there's also tokio::time::Sleep now, which I guess works similarly but probably uses some Runtime Handle instead.

looking at the Sleeping state, there might be a case were the Future returns Pending and gets not woken up again (elpased < polling_interval)

tokio::time::Sleep won't work in WASM iirc

I believe all common executors schedule futures for polling in frames regardless of whether the waker has explicitly requested waking

didn't finish the previous thought before submitting the tx

given that this could cause issues with uncommon executors, should we audit all our futures to ensure that all polls either request rewaking or poll another future?

mattsse

I feel like this could even be two different types.
An additional Stream type that processes all transactions.
The EscalatingPending Future that polls the underlying stream and returns the first value?

mattsse · 2021-11-08T23:36:54Z

ethers-providers/src/pending_escalator.rs

+                        completed!(this, Ok(receipt));
+                    }
+                    // rewake until drained
+                    Poll::Ready(Ok(None)) => cx.waker().wake_by_ref(),


this section looks like it could be wrapped in a loop over all futures? So that Poll::Ready(Ok(None)) => merely results in a continue?
Is the order in which they should be polled here important, if not then Vec::swap_remove could be useful.

…ne again

prestwich · 2021-11-09T17:58:57Z

test failure is unrelated to these changes

@gakonst @mattsse do you think that this belongs on the main middleware or on some sort of extension trait?

mattsse · 2021-11-09T18:14:39Z

@gakonst @mattsse do you think that this belongs on the main middleware or on some sort of extension trait?

What, would be nice is if this could be somehow integrated with the current send_transaction -> PendingTx, so that we can do something like p.send_transaction(tx).escalate(||...).await but p.send_transaction(tx).await.await still works.

I believe this would be possible with an additional future type (SendTx or something?) that would have Output = PendingTransaction and a fn escalate. But this would mean, async fn send_transaction would become fn send_transaction() -> SendTx.

this would be a breaking change however.

The easiest way would probably be to add another provider function, as is the case now.

prestwich · 2021-11-09T18:22:57Z

the problem with that is that the signing must be done in the top-level middleware. It can't be done by the provider that the pending transaction has access to. So that feature is blocked by an object-safe Middleware trait

mattsse · 2021-11-09T18:38:45Z

ah I see, makes sense, this would then be limited to only the SignerMiddleware...

prestwich · 2021-11-09T18:42:11Z

Not exactly, as the raw provider may sign in many cases (using eth_sign or similar). However, relying on the provider would result in a conflict if any SignerMiddleware were used.

E.g SignerMiddleware( Provider ). The SignerMiddleware may be signing with address 0x1234... where the node controls address 0xabcd.... This would cause errors and other unexpected behavior if we choose to have signing in the Provider held by a PendingTransaction

gakonst

looking good, I am OK with placing it as an extra function as-is. I also liked @mattsse's idea of adding an extra type on top of PendingTransaction so that we can do send_transaction(tx).await?.escalator(...).await? to escalate and send_transaction(tx).await?.await? for simple usage

gakonst · 2021-11-10T17:02:39Z

ethers-providers/src/pending_escalator.rs

+    }
+
+    pub fn broadcast_interval(mut self, duration: u64) -> Self {
+        self.broadcast_interval = Duration::from_secs(duration);


Let's use milliseconds here, just for flexbility sake

Suggested change

self.broadcast_interval = Duration::from_secs(duration);

self.broadcast_interval = Duration::from_millis(duration);

going to change it to use Into<Duration> to match the behavior of PendingTransaction

gakonst · 2021-11-10T17:06:56Z

ethers-providers/src/pending_escalator.rs

+    }
+
+    pub fn polling_interval(mut self, duration: u64) -> Self {
+        self.polling_interval = Duration::from_secs(duration);


Suggested change

self.polling_interval = Duration::from_secs(duration);

self.polling_interval = Duration::from_millis(duration);

gakonst · 2021-11-10T18:20:31Z

ethers-providers/src/pending_escalator.rs

+    }
+
+    pub fn polling_interval(mut self, duration: u64) -> Self {
+        self.polling_interval = Duration::from_secs(duration);


Suggested change

self.polling_interval = Duration::from_secs(duration);

self.polling_interval = Duration::from_millis(duration);

gakonst · 2021-11-10T18:24:51Z

ethers-providers/src/pending_escalator.rs

+                        completed!(this, Ok(receipt));
+                    }
+                    // rewake until drained
+                    Poll::Ready(Ok(None)) => cx.waker().wake_by_ref(),


Looping over all futures is an anti-pattern I think, the state machine may be slightly easier to write but as @prestwich says it's better to do it on a per-fut basis

prestwich · 2021-11-10T22:15:37Z

latest commit uses FuturesUnordered. I think this is better as @mattsse says. We don't require ordering here, so this functions similarly to select_all, but is easier to manage

prestwich · 2021-11-10T22:16:07Z

I have no other outstanding changes so I'll update the changelog and this is ready for consideration

gakonst

I like it. Thank you for the hard work, we can iterate on it post-merge. @mattsse any further comments?

ethers-providers/src/pending_escalator.rs

mattsse · 2021-11-11T19:10:19Z

I like it. Thank you for the hard work, we can iterate on it post-merge. @mattsse any further comments?

lgtm

gakonst · 2021-11-12T01:23:57Z

gg. Should we receive the escalator middleware?

Same reasoning as gakonst#566

luketchang reviewed Nov 8, 2021

View reviewed changes

gakonst reviewed Nov 8, 2021

View reviewed changes

prestwich commented Nov 8, 2021

View reviewed changes

gakonst marked this pull request as ready for review November 8, 2021 21:36

mattsse reviewed Nov 8, 2021

View reviewed changes

prestwich added 7 commits November 9, 2021 09:22

feature: pending_escalator

e3a3713

feature: send_escalating in Middleware

8dd1a18

bug: don't drop unready futures in escalator.poll

533b59d

chore: docs and must_use

0e3318a

chores: lints, clippies, wasm fixes, dedup pinboxfut

29eb74c

chore: more lints

44fb4ce

refactor: use Delay in polling interval to ensure re-waking

a832b48

prestwich force-pushed the prestwich/super-pending branch from 76e79e2 to a832b48 Compare November 9, 2021 17:22

prestwich added 3 commits November 9, 2021 09:38

refactor: simplify Sleeping state transition as last will never be No…

9d6be17

…ne again

bug: properly set last when broadcasts resolve

d970471

feature: debug implementation for EscalatingPending

1afd9c9

gakonst reviewed Nov 10, 2021

View reviewed changes

prestwich added 2 commits November 10, 2021 13:45

refactor: with_ setters and escalations argument

ed25721

refactor: use FuturesUnOrdered instead of a vec of futures

7b91c26

chore: update CHANGELOG with recent PR info

096ca41

prestwich requested a review from gakonst November 10, 2021 22:18

gakonst approved these changes Nov 11, 2021

View reviewed changes

ethers-providers/src/pending_escalator.rs Outdated Show resolved Hide resolved

prestwich added 2 commits November 11, 2021 11:34

chore: update all comments on pending escalator

a001551

chore: run rustfmt

67a9e15

gakonst merged commit 203b2e8 into master Nov 12, 2021

gakonst deleted the prestwich/super-pending branch November 12, 2021 01:23

meetmangukiya pushed a commit to meetmangukiya/ethers-rs that referenced this pull request Mar 21, 2022

Checkout sources in prepare step (gakonst#566)

bbf3273

meetmangukiya pushed a commit to meetmangukiya/ethers-rs that referenced this pull request Mar 21, 2022

Checkout sources in release cleanup (gakonst#573)

d155606

Same reasoning as gakonst#566

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prestwich/super pending #566

Prestwich/super pending #566

prestwich commented Nov 8, 2021 •

edited

Loading

luketchang Nov 8, 2021

prestwich Nov 8, 2021

luketchang Nov 8, 2021

prestwich Nov 8, 2021

mattsse Nov 8, 2021

prestwich Nov 9, 2021

gakonst Nov 10, 2021

mattsse Nov 10, 2021

gakonst left a comment

gakonst Nov 8, 2021

prestwich Nov 8, 2021

prestwich Nov 8, 2021

mattsse Nov 8, 2021

prestwich Nov 9, 2021

prestwich Nov 9, 2021

mattsse left a comment

mattsse Nov 8, 2021

prestwich commented Nov 9, 2021

mattsse commented Nov 9, 2021 •

edited

Loading

prestwich commented Nov 9, 2021

mattsse commented Nov 9, 2021

prestwich commented Nov 9, 2021

gakonst left a comment

gakonst Nov 10, 2021

prestwich Nov 10, 2021

gakonst Nov 10, 2021

gakonst Nov 10, 2021

gakonst Nov 10, 2021

prestwich commented Nov 10, 2021

prestwich commented Nov 10, 2021

gakonst left a comment

mattsse commented Nov 11, 2021

gakonst commented Nov 12, 2021

	self.broadcast_interval = Duration::from_secs(duration);
	self.broadcast_interval = Duration::from_millis(duration);

	self.polling_interval = Duration::from_secs(duration);
	self.polling_interval = Duration::from_millis(duration);

Prestwich/super pending #566

Prestwich/super pending #566

Conversation

prestwich commented Nov 8, 2021 • edited Loading

Motivation

Solution

things I don't like

PR Checklist

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

gakonst left a 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

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestwich commented Nov 9, 2021

mattsse commented Nov 9, 2021 • edited Loading

prestwich commented Nov 9, 2021

mattsse commented Nov 9, 2021

prestwich commented Nov 9, 2021

gakonst left a 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

prestwich commented Nov 10, 2021

prestwich commented Nov 10, 2021

gakonst left a comment

Choose a reason for hiding this comment

mattsse commented Nov 11, 2021

gakonst commented Nov 12, 2021

prestwich commented Nov 8, 2021 •

edited

Loading

mattsse commented Nov 9, 2021 •

edited

Loading