bee icon indicating copy to clipboard operation
bee copied to clipboard

feat: threshold upgrades and request ahead-of-time

Open metacertain opened this issue 2 years ago • 1 comments

Note: this is a semi-breaking change, making light nodes with old versions unable to connect to this version, as the old minimum payment threshold expectation wouldn't be met anymore by the threshold given by new full nodes. To be merged & released with more breaking changes

Changes part 1

This PR introduces a mechanism, that favors long-standing connections by gradually upgrading payment thresholds after an amount of traffic is served for and settled by a peer.

The milestone for upgrading a payment threshold is after each 100 seconds worth of refreshment rate being settled (by either time based or monetary settlements). The upgrade of the payment threshold is an increase of 1 second worth of refreshment rate per upgrade. After the first 18 milestones the increase slows down to a logarithmic rate, by setting the next milestone at twice of the current milestones accumulated traffic settled.

When a peer is newly connected, it receives a default payment threshold once again, so that disconnections / reconnections are not incentivized.

The PR introduces individual payment thresholds given to peers. The PR also includes relaying the information about whether a peer is a full node to accounting from pseudosettlements, in order to differentiate traffic milestones for threshold upgrades between light nodes and full nodes. The PR introduces differentiation of initial payment thresholds given to light nodes and full nodes.

Changes part 2:

Decrease default payment thresholds to 10%

Changes part 3:

in order to counteract queue throttling effects of requests served with delays, increase reserve and disconnect limits from payment threshold, (1 + tolerance%) * payment threshold

to

payment threshold + ("seconds since last refreshment sent" * "refreshment rate" ) (1 + tolerance%) * payment threshold + ("seconds since last refreshment received" * "refreshment rate")

with "seconds since last refreshment" term capped at 1.

this intends to circumvent a situation where reserve is jammed by timing out requests, and no debt is formed, so no more requests could be queried regardless of refresh rate.

Changes part 4:

hotfix for NTP differences related problems resulting in recipient of a refreshment refusing refreshment as it is "too soon"


This change is Reviewable

metacertain avatar Apr 26 '22 07:04 metacertain

looks good, but I find the time representations here confusing. They are represented with various types which makes a lot of overhead of reading but also interacting with the values. Not sure why the standard-library types (time.Time, time.Duration) weren't used and I think that leveraging them would make this much easier and nicer to read.

Reviewed 23 of 23 files at r2, all commit messages. Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @metacertain)

pkg/accounting/accounting.go line 270 at r2 (raw file):

	}

	timeElapsed := (a.timeNow().UnixMilli() - accountingPeer.refreshTimestamp) / 1000

I find this a bit confusing and not particularly readable. There are the time.Time and time.Duration types, why not use them?

attempted to disambiguate by naming

pkg/accounting/accounting.go line 275 at r2 (raw file):

	}

	refreshDue := new(big.Int).Mul(big.NewInt(timeElapsed), a.refreshRate)

is big.Int needed to represent time? I believe that even the standard library represents time.Time in nanoseconds unix epoch with uint64. Again using the standard library native types here would be more readable and clear

big int conversion is necessary for input for big int operations

pkg/accounting/accounting.go line 417 at r2 (raw file):

	// Don't do anything if there is no actual debt or no time passed since last refreshment attempt
	// This might be the case if the peer owes us and the total reserve for a peer exceeds the payment threshold.
	if paymentAmount.Cmp(new(big.Int).Mul(a.refreshRate, big.NewInt(2))) >= 0 {

significant change here with no comment update

added comment

pkg/accounting/accounting.go line 418 at r2 (raw file):

	// This might be the case if the peer owes us and the total reserve for a peer exceeds the payment threshold.
	if paymentAmount.Cmp(new(big.Int).Mul(a.refreshRate, big.NewInt(2))) >= 0 {
		if timeElapsed > 999 {

this is obscure and needs a comment on why it is necessary

added comment

pkg/accounting/accounting.go line 466 at r2 (raw file):

		if a.payFunction != nil && !balance.paymentOngoing {

			difference := now/1000 - balance.lastSettlementFailureTimestamp

more complexity relating to time unit conversions. at this point it is getting repeated and unclear which units what expects, what are the tolerances, etc. using time primitives will improve this significantly

attempted to disambiguate by naming

pkg/accounting/accounting.go line 467 at r2 (raw file):

			difference := now/1000 - balance.lastSettlementFailureTimestamp
			if difference > failedSettlementInterval {

more of the same soup....

attempted to disambiguate by naming

pkg/accounting/accounting.go line 593 at r2 (raw file):

			paymentThresholdForPeer: new(big.Int).Set(a.paymentThreshold),
			disconnectLimit:         new(big.Int).Set(a.disconnectLimit),
			thresholdGrowAt:         new(big.Int).Set(a.thresholdGrowStep),

don't understand if this signifies a point in time or a threshold size step increase. type confusion

left as is for now

pkg/node/node.go line 174 at r2 (raw file):

const (
	refreshRate                   = int64(4500000)
	lightFactor                   = 10

a comment on what this means would be nice

added comments

pkg/node/node.go line 667 at r2 (raw file):

	}

	minThreshold := big.NewInt(2 * refreshRate)

these are also quite obscure. having more comments that would explain what this means and what are the repercussions would be nice

yes this is not so obvious why necessary, also left as is now and to be cleaned up in a separate PR

metacertain avatar Jun 02 '22 08:06 metacertain