Skip to content
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.

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

feat: Maintain originated balance #1870

Merged
merged 1 commit into from
Jun 11, 2021
Merged

feat: Maintain originated balance #1870

merged 1 commit into from
Jun 11, 2021

Conversation

metacertain
Copy link
Member

@metacertain metacertain commented May 25, 2021

The aim of this change is to introduce originated balances, that keep track of credit operations done for the purpose of requests originated by the node itself, and to limit monetary payments to the debt showed by the originated balance.

This balance changes in the following ways:

	- to keep track of originated credits
		+ it decreases with the value of these credit operations
	- to be limited by the current outstanding debt to our peer shown by the accounting balance, 
		+ when it is negative it increases to the value of the current accounting balance when the accounting balance increases to a negative value closer to or above zero, this increase can not bring the originated balance above 0
	- to reflect monetary payments made with the sole purpose of compensating for the originated balance,
		+ it increases by the amount payed for by swap settlements, possibly bringing the originated balance above 0.
		+ when originated balance is above 0 it can only decrease by originated credit operations, so if by chance we have "overpayed" for this originated balance, the surplus amount is still counted as originated traffic used after the payment happened

In accounting, the following changes implement this:

  • When credit is done, if the originated argument is true, we deduct the price from the originated balance

  • When settle (time based part) or debit decreases negative balance, originated balance is reduced towards zero along with the normal balance if the negative balance would become closer to zero.

  • When notifyPaymentSent decreases negative balance, the value of the settlement is added to the originated balance, even increasing it's value above zero (as the payment was solely intended to move the originated balance).

  • When sending monetary payments limiting amount to current outstanding originated debt

The PR also includes the use of originated parameter to credit in protocols


This change is Reviewable

@metacertain metacertain changed the base branch from master to retrieval_behaviour May 25, 2021 16:50
Copy link
Member

@zelig zelig left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @metacertain)


pkg/accounting/accounting.go, line 29 at r1 (raw file):

	balancesPrefix           string    = "accounting_balance_"
	balancesSurplusPrefix    string    = "accounting_surplusbalance_"
	balancesOriginatedPrefix string    = "accounting_originatedbalance_"

the word originated is enigmatic. can we use a descriptive term?

@metacertain
Copy link
Member Author

metacertain commented May 31, 2021

the word originated is enigmatic. can we use a descriptive term?

we concluded with @ralph-pichler this is a descriptive term, as this is the balance representing originated traffic.
however if there is a better suggestion, we can refactor

Copy link
Member

@zelig zelig left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @metacertain)

@acud
Copy link
Member

acud commented Jun 4, 2021

@metacertain still not really clear to me what originated means in this sense here. there's no linked issue, no description of the term, i agree with viktor that this is just too obscure and not descriptive. can you please explain what it means?

@metacertain
Copy link
Member Author

@metacertain still not really clear to me what originated means in this sense here. there's no linked issue, no description of the term, i agree with viktor that this is just too obscure and not descriptive. can you please explain what it means?

updated pr description with explanation of changes

@metacertain metacertain force-pushed the retrieval_behaviour branch 2 times, most recently from 89f551e to fad6396 Compare June 10, 2021 13:14
Copy link
Contributor

@mrekucci mrekucci left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 6 files at r1, 4 of 4 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @metacertain)


pkg/accounting/accounting.go, line 27 at r3 (raw file):

var (
	_                        Interface = (*Accounting)(nil)
	balancesPrefix           string    = "accounting_balance_"

The string type can be omitted (also for the vars below) and the related variables grouped.


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

	//

	if originated {

This can be rewritten in early return which would make the code more readable:

if !originated {
    return nil
}

...

pkg/accounting/accounting.go, line 277 at r3 (raw file):

	if originated {
		originBalance, err := a.OriginatedBalance(peer)
		if err != nil {

No need for double if, just if err != nil && !errors.Is(err, ErrPeerNoBalance)... will do.


pkg/accounting/accounting.go, line 360 at r3 (raw file):

		err = a.decreaseOriginatedBalanceTo(peer, oldBalance)
		if err != nil {
			a.logger.Warningf("settle: failed to decrease originated balance: %w", err)

The %w wouldn't work in this context, switch to %v


pkg/accounting/accounting.go, line 468 at r3 (raw file):

func originatedBalanceKey(peer swarm.Address) string {
	return fmt.Sprintf("%s%s", balancesOriginatedPrefix, peer.String())

The String() method call on the peer is unnecessary since %s formatting directive will do the same.


pkg/accounting/accounting.go, line 704 at r3 (raw file):

	err = a.decreaseOriginatedBalanceBy(peer, amount)
	if err != nil {
		a.logger.Warningf("accounting: notifypaymentsent failed to decrease originated balance: %w", err)

As above, replace the %w with %v.


pkg/accounting/accounting.go, line 904 at r3 (raw file):

	err = a.decreaseOriginatedBalanceTo(peer, nextBalance)
	if err != nil {
		a.logger.Warningf("increase balance: failed to decrease originated balance: %w", err)

As above, replace the %w with %v.


pkg/accounting/accounting.go, line 953 at r3 (raw file):

func (a *Accounting) decreaseOriginatedBalanceTo(peer swarm.Address, limit *big.Int) error {

	zero := big.NewInt(0)

This is unnecessary since you've already declared the zero value above.


pkg/accounting/accounting.go, line 958 at r3 (raw file):

	originatedBalance, err := a.OriginatedBalance(peer)
	if err != nil {

As above, this can be simplified into just one if statement.


pkg/accounting/accounting.go, line 984 at r3 (raw file):

	originatedBalance, err := a.OriginatedBalance(peer)
	if err != nil {

As above, this can be simplified into just one if statement.


pkg/accounting/metrics.go, line 43 at r3 (raw file):

			Help:      "Amount of BZZ credited to peers (potential cost of the node)",
		}),
		TotalOriginatedCreditedAmount: prometheus.NewCounter(prometheus.CounterOpts{

This can follow the position defined in the struct and so be moved to the penultimate place.


pkg/accounting/metrics.go, line 61 at r3 (raw file):

			Help:      "Number of occurrences of BZZ credit events towards peers",
		}),
		OriginatedCreditEventsCount: prometheus.NewCounter(prometheus.CounterOpts{

This can follow the position defined in the struct and so be moved to the last place.

@metacertain
Copy link
Member Author

attended all comments as suggested, except

The String() method call on the peer is unnecessary since %s formatting directive will do the same.

this lead to unexplained symptoms in the past, wouldn't try it at the moment

func (a *Accounting) decreaseOriginatedBalanceTo(peer swarm.Address, limit *big.Int) error {

zero := big.NewInt(0)

This is unnecessary since you've already declared the zero value above.

we removed global zero as a risk factor, it could be changed by functions receiving it

Copy link
Contributor

@mrekucci mrekucci left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @metacertain)

@@ -42,7 +43,7 @@ type Interface interface {
// Release releases the reserved funds.
Release(peer swarm.Address, price uint64)
// Credit increases the balance the peer has with us (we "pay" the peer).
Credit(peer swarm.Address, price uint64) error
Credit(peer swarm.Address, price uint64, orignated bool) error
Copy link
Member Author

Choose a reason for hiding this comment

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

typo

Base automatically changed from retrieval_behaviour to master June 11, 2021 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull-request ready for review The PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants