Skip to content

fix: period index calculation #1641

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jun 9, 2025
Merged

fix: period index calculation #1641

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
06ee3b5
fix: period index calculation
hopeyen Jun 4, 2025
ffb047a
refactor: accountant test structuring
hopeyen Jun 6, 2025
978621c
docs: code comments and err msgs
hopeyen Jun 6, 2025
20cd495
refactor: rm comment
hopeyen Jun 9, 2025
90617e0
fix: numBins, private accessor, docs
hopeyen Jun 9, 2025
dac3944
Merge branch 'master' into hope/accountant-circular-period-records
hopeyen Jun 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 24 additions & 9 deletions api/clients/v2/accountant.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ type Accountant struct {
minNumSymbols uint64

// local accounting
// contains 3 bins; circular wrapping of indices
// contains numBins of period records; circular wrapping on period record indices
periodRecords []PeriodRecord
usageLock sync.Mutex
cumulativePayment *big.Int
Expand All @@ -34,8 +34,11 @@ type Accountant struct {
numBins uint32
}

// PeriodRecord contains the index of the reservation period and the usage of the period
type PeriodRecord struct {
// Index is start timestamp of the period in seconds; it is always a multiple of the reservation window
Index uint32
// Usage is the usage of the period in symbols
Usage uint64
}

Expand Down Expand Up @@ -73,13 +76,13 @@ func (a *Accountant) BlobPaymentInfo(
numSymbols uint64,
quorumNumbers []uint8,
timestamp int64) (*big.Int, error) {

currentReservationPeriod := meterer.GetReservationPeriodByNanosecond(timestamp, a.reservationWindow)
reservationWindow := a.reservationWindow
currentReservationPeriod := meterer.GetReservationPeriodByNanosecond(timestamp, reservationWindow)
symbolUsage := a.SymbolsCharged(numSymbols)

a.usageLock.Lock()
defer a.usageLock.Unlock()
relativePeriodRecord := a.GetRelativePeriodRecord(currentReservationPeriod)
relativePeriodRecord := a.GetOrRefreshRelativePeriodRecord(currentReservationPeriod, reservationWindow)
relativePeriodRecord.Usage += symbolUsage

// first attempt to use the active reservation
Expand All @@ -91,7 +94,7 @@ func (a *Accountant) BlobPaymentInfo(
return big.NewInt(0), nil
}

overflowPeriodRecord := a.GetRelativePeriodRecord(currentReservationPeriod + 2)
overflowPeriodRecord := a.GetOrRefreshRelativePeriodRecord(currentReservationPeriod+2*reservationWindow, reservationWindow)
// Allow one overflow when the overflow bin is empty, the current usage and new length are both less than the limit
if overflowPeriodRecord.Usage == 0 && relativePeriodRecord.Usage-symbolUsage < binLimit && symbolUsage <= binLimit {
if err := QuorumCheck(quorumNumbers, a.reservation.QuorumNumbers); err != nil {
Expand All @@ -116,8 +119,8 @@ func (a *Accountant) BlobPaymentInfo(
return a.cumulativePayment, nil
}
return big.NewInt(0), fmt.Errorf(
"no bandwidth reservation found for account %s, and current cumulativePayment balance insufficient "+
"to make an on-demand dispersal. Consider depositing more eth to the PaymentVault contract.", a.accountID.Hex())
"invalid payments: no available bandwidth reservation found for account %s, and current cumulativePayment balance insufficient "+
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really feel like this function should be broken into two: a function that only checks (and updates) reservation, and IF that fails (returns nil), then its caller's job to get a cumulativePayment). This function is very hard to review because its doing too many things, and BlobPaymentInfo doesn't reflect the fact that it updates state, and possibly returns a cumulativePayment. Can it update reservation state when returning a nonzero cumulativePayment?

These are all questions (and many more) that are running through my brain as I'm reviewing this. We could write more documentation on the function to describe all these possible scenarios, but I feel like it would be much simpler (and self-documenting) to split into 2 functions. Can prob be done in a separate PR at a future date, but still wanted to leave this comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good refactor, only called in one spot func (a *Accountant) AccountBlob.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with the reasoning. This is something I have planned in this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this is just a refactor, could you make it a separate in-between PR, to not bloat the PR with actual new features?
In integration team, we try (but tbf its hard) to follow the RII pattern: new feature introductions should be broken into 3 separate PRs:

  1. refactor (any code refactors that are needed to prepare for the feature addition)
  2. interface
  3. implementation

You already broke your PR into interface and implementation, so that's awesome.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I can break it up. Given this is meant to be a fix for the current accounting, I would do refactor/interface/implementation for the upcoming quorum specific accounting

"to make an on-demand dispersal. Consider increasing reservation or cumulative payment on-chain. For more details, see https://docs.eigenda.xyz/core-concepts/payments#disperser-client-requirements", a.accountID.Hex())
}

// AccountBlob accountant provides and records payment information
Expand Down Expand Up @@ -157,9 +160,21 @@ func (a *Accountant) SymbolsCharged(numSymbols uint64) uint64 {
return core.RoundUpDivide(numSymbols, a.minNumSymbols) * a.minNumSymbols
}

// GetRelativePeriodRecord returns the period record for the given index
func (a *Accountant) GetRelativePeriodRecord(index uint64) *PeriodRecord {
relativeIndex := uint32(index % uint64(a.numBins))
if a.periodRecords[relativeIndex].Index != uint32(index) {
relativeIndex := uint32((index / a.reservationWindow) % uint64(a.numBins))
// Return empty record if the index is greater than the number of bins (should never happen by accountant initialization)
if relativeIndex >= uint32(a.numBins) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it ever possible that a.numBins can change at runtime?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no we don't have mut on it, it will be fixed to the configured value

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wdym by "have mut"? A new developer could certainly add some code that changes it (by mistake). Would that be a problem? If yes, then it should be documented in the accountant struct.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking of which.. how should one choose numBins? There is no comment as to this effect, neither on the struct nor on the constructor.

  1. can it be anything?
  2. does it have to align with the disperser's meterer? (I see on our docs that "The disperser will track at least 4 periods per reservation, starting from the previous period to the period after next period. "). are these period per reservation the same thing?
  3. what is a good choice for this number? why? does it even matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added some documentation.

  1. Anything. the accountant would use that configured number or 3, whichever is bigger
  2. the disperser tracks the same thing
  3. 3 is the minimum and the default to account for overflow usage. In reality client could even just have one bin and doesn't provide overflow functionality within the accountant. Using a bigger number doesn't provide functional difference to the accountant, but the dispersal client could track more reservation history

Copy link
Collaborator

@samlaf samlaf Jun 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. how do you make sure the client is using the same numBins as the disperser? EDIT: I see you overwrite PeriodRecords in SetPaymentState... could we do this in constructor instead? SetPaymentState is mandatory to be called to function properly right? This should prob be documented, and also AccountBlob should just panic if SetPaymentState has not been called I think?
  2. whats the point of allowing >3 then if there's no functional difference? whats the point of tracking reservation history?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raised a similar question here #1599 (comment). Let's move the discussion to that PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. numBins doesn't have to use the same numBins as the disperser. SetPaymentState are only called by the first dispersal request if the accountant isn't filled in. The idea was to allow clients to implement accountants that manage state differently such as persisting them instead of relying on the disperser to provide the payment state, or call eth client directly for the onchain states. These are thing we won't implement, but allowing the possibility for modifications. Perhaps we should make it more of a requirement. Can keep this thought for later

  2. I figured some clients might want to track more history for analyzing and adjusting their usage. The circular wrapping invariants stay the same with >3, and not a requirement to configure, so I figured it won't hurt. We can iterate on whether clients should have this flexibility

panic(fmt.Sprintf("relativeIndex %d is greater than the number of bins %d cached", relativeIndex, a.numBins))
}
return &a.periodRecords[relativeIndex]
}

// GetOrRefreshRelativePeriodRecord returns the period record for the given index (which is in seconds and is the multiple of the reservation window),
// wrapping around the circular buffer and clearing the record if the index is greater than the number of bins
func (a *Accountant) GetOrRefreshRelativePeriodRecord(index uint64, reservationWindow uint64) *PeriodRecord {
relativeIndex := uint32((index / reservationWindow) % uint64(a.numBins))
if a.periodRecords[relativeIndex].Index < uint32(index) {
a.periodRecords[relativeIndex] = PeriodRecord{
Index: uint32(index),
Usage: 0,
Expand Down
Loading
Loading