server/swap: metered block-triggered conf and penalty checks #1629

buck54321 · 2022-05-26T14:20:22Z

Ethereum has very short block times. And just for conservation of
resources, tightly-grouped blocks need not trigger tightly-grouped
conf and penalty checks. This adds a minimum delay window to block
notifications. If blocks comes farther than the minimum delay apart,
nothing changes from the current behavior. If 1 or more blocks
arrives within the delay window of another, they will be collected into a
single block notification that occurs once the min delay has passed.

The newly implemented SignalMeterer is only used in the swapper for
block notifications for now, but might also be useful for the client.

chappjc · 2022-05-26T16:26:51Z

Cool. I guess I should get #1563 out of draft. Lock contention can be just as bad as blocking the ws inHandler or backing up conf checks depending on where it is blocking.

LasTshaMAN

Minor things

LasTshaMAN · 2022-05-28T19:10:54Z

dex/meter/meterer.go

+	if s.scheduled != nil {
+		if !s.scheduled.Stop() {
+			<-s.scheduled.C
+		}
+	}


maybe also need to s.scheduled = nil inside of this if (assuming Stop method is only meant to cancels any scheduled signals and not completely prevent further usage of SignalMeterer),

as the following condition will be hit, it seems:

and not completely prevent further usage of SignalMeterer

I always just assumed this was a one-Stop type.

yeah, that would be the default assumption for me as well,

just thought comment could be more precise (maybe this code can be re-used in other places as well in the future)

Stop further signalling and cancel any scheduled signals.

LasTshaMAN · 2022-05-28T19:17:49Z

server/swap/swap.go

@@ -34,6 +35,7 @@ var (
 	// txWaitExpiration is the longest the Swapper will wait for a coin waiter.
 	// This could be thought of as the maximum allowable backend latency.
 	txWaitExpiration = 2 * time.Minute
+	minBlockPeriod   = time.Second * 10


might be useful to describe how/why this value was chosen

I really pulled it out of thin air. The value is open for debate. Anybody want a different value?

maybe just some explanation regarding why this delay is needed in the first place would be nice to have (idk, one can search through git history and find this PR with the answer as well),

can suggest something like:

// minBlockPeriod helps with limiting notification bursts when blocks are generated too fast (e.g. in Ethereum occasionally several blocks can are generated in a matter of several seconds).

Agree a sentence would be good here.

As far as the choice of 10s goes, I suppose that's short enough that it would not be an annoying delay, but also long enough that checks shouldn't pile up too much. (it works for me)

JoeGruffins · 2022-05-30T05:03:07Z

dex/meter/meterer.go

+// the previous signal, that signal will be scheduled for later to respect the
+// configured delay. If multiple signals arrive within minDelay, they will be
+// grouped into a single delayed signal. Error signals are never delayed.
+type SignalMeterer struct {


Because you have NewSignalMeterer this does not need to be exported maybe?

I don't think so. It's used as a concrete type by an importing package. I think it's customary to export it.

Yes agree with @buck54321 . Some linters will even nag if an exported constructor returns and unexported type because it can be "annoying to use" or something to that extent.

Thanks. Understood.

chappjc

Cool little utility. Overall seems like good logic and it works.

I don't totally love the goroutine use as a means to get unlimited buffering courtesy of the runtime, and I have some ideas there, but nothing that really has to change.

chappjc · 2022-06-07T21:11:33Z

dex/meter/meterer.go

+	defer s.mtx.Unlock()
+	// Errors are passed immediately.
+	if err != nil {
+		go s.recv(err)


I think you might as well replace recv func(error) with recv chan error, and doing s.recv <- err instead of go s.recv(err).

The goroutine allocations and runtime overhead seem avoidable, and it feels tailored for a channel anyway. I get the sense you're using goroutines as a way to use the runtime for effectively unlimited buffering, which could backfire in addition to being a bit inefficient.

Plus in Swapper, I think a channel is more natural since you've got a select loop going already. illustration: 124ec64

And as usual lately I'm looking at patterns that eliminate locks and fields with event loops and closure, so I'm toying with this alternate mechanism: 817b800
Just toying around, don't feel like you need to take either suggestion.

817b800 is beautiful 😍.

chappjc · 2022-06-07T21:12:08Z

dex/meter/meterer.go

+	s.mtx.Lock()
+	defer s.mtx.Unlock()
+	// Errors are passed immediately.
+	if err != nil {


Can move the locking until after this first early return

chappjc · 2022-06-07T21:29:37Z

server/swap/swap.go

+			// TODO: This pattern would still send the block trigger half of the
+			// time if the ctxMaster is canceled.


Good catch. That's annoying. Seems harmless, just not really intended.

chappjc · 2022-06-07T21:33:51Z

server/swap/swap.go

@@ -34,6 +35,7 @@ var (
 	// txWaitExpiration is the longest the Swapper will wait for a coin waiter.
 	// This could be thought of as the maximum allowable backend latency.
 	txWaitExpiration = 2 * time.Minute
+	minBlockPeriod   = time.Second * 10


Agree a sentence would be good here.

As far as the choice of 10s goes, I suppose that's short enough that it would not be an annoying delay, but also long enough that checks shouldn't pile up too much. (it works for me)

Ethereum has very short block times. And just for conservation of resources, tightly-grouped blocks need not trigger tightly-grouped conf and penalty checks. This adds a minimum delay window to block notifications. If blocks comes farther than the minimum delay apart, nothing changes from the current behavior. If 1 or more blocks arrives within the delay window of another, they will be collected into a single block notification that occurs once the min delay has passed. The newly implemented DelayedRelay is only used in the swapper for block notifications for now, but might also be useful for the client. Co-authored-by: Jonathan Chappelow <chappjc@gmail.com>

buck54321 changed the title ~~server/swap: metered block-triggered swap confirmation tests~~ server/swap: metered block-triggered conf and penalty checks May 26, 2022

LasTshaMAN reviewed May 28, 2022

View reviewed changes

JoeGruffins approved these changes May 30, 2022

View reviewed changes

chappjc self-requested a review June 7, 2022 16:35

chappjc requested changes Jun 7, 2022

View reviewed changes

chappjc added this to the 0.5 milestone Jun 9, 2022

buck54321 force-pushed the metered-blocks branch from 53554f2 to b1a1bd1 Compare June 14, 2022 23:55

chappjc force-pushed the metered-blocks branch from 59f17c9 to 8ff2de1 Compare June 16, 2022 02:46

chappjc merged commit c2b380d into decred:master Jun 16, 2022

		// TODO: This pattern would still send the block trigger half of the
		// time if the ctxMaster is canceled.

server/swap: metered block-triggered conf and penalty checks #1629

server/swap: metered block-triggered conf and penalty checks #1629

Uh oh!

Conversation

buck54321 commented May 26, 2022

Uh oh!

chappjc commented May 26, 2022

Uh oh!

LasTshaMAN left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

buck54321 May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LasTshaMAN May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

chappjc May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chappjc 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!

Uh oh!

buck54321 May 31, 2022 •

edited

Loading

LasTshaMAN May 31, 2022 •

edited

Loading

chappjc May 31, 2022 •

edited

Loading