bee
bee copied to clipboard
feat: threshold upgrades and request ahead-of-time
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"
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
andtime.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 representstime.Time
in nanoseconds unix epoch withuint64
. 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