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: avoid tracking unbounded balances #1665

Merged
merged 10 commits into from
Sep 4, 2024
18 changes: 4 additions & 14 deletions components/ledger/internal/engine/command/commander.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import (
"github.com/formancehq/ledger/internal/bus"
"github.com/formancehq/ledger/internal/engine/utils/batching"
"github.com/formancehq/ledger/internal/machine/vm"
"github.com/formancehq/stack/libs/go-libs/collectionutils"
"github.com/formancehq/stack/libs/go-libs/metadata"
"github.com/pkg/errors"
)
Expand Down Expand Up @@ -126,22 +125,13 @@ func (commander *Commander) exec(ctx context.Context, parameters Parameters, scr
return nil, NewErrCompilationFailed(err)
}

involvedAccounts, involvedSources, err := func() ([]string, []string, error) {
involvedAccounts, involvedSources, err := m.ResolveResources(ctx, commander.store)
if err != nil {
return nil, nil, NewErrCompilationFailed(err)
}

return involvedAccounts, involvedSources, nil
}()
readLockAccounts, writeLockAccounts, err := m.ResolveResources(ctx, commander.store)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: this is the only usage of the ResolveResources method

if err != nil {
return nil, err
return nil, NewErrCompilationFailed(err)
}

worldFilter := collectionutils.FilterNot(collectionutils.FilterEq("world"))
lockAccounts := Accounts{
Read: collectionutils.Filter(involvedAccounts, worldFilter),
Write: collectionutils.Filter(involvedSources, worldFilter),
Read: readLockAccounts,
Write: writeLockAccounts,
}

unlock, err := func() (Unlock, error) {
Expand Down
47 changes: 33 additions & 14 deletions components/ledger/internal/machine/script/compiler/compiler.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,15 @@ type parseVisitor struct {
varIdx map[string]machine.Address
// needBalances store for each account, the set of assets needed
neededBalances map[machine.Address]map[machine.Address]struct{}

// The sources accounts that aren't unbounded
// that is, @world or sources that appear within a
// '.. allowing unboundeed overdraft' clause
writeLockAccounts map[machine.Address]struct{}

// all the accounts that appear in either the destination
// or in the balance() function
readLockAccounts map[machine.Address]struct{}
}

// Allocates constants if it hasn't already been,
Expand Down Expand Up @@ -580,6 +589,7 @@ func (p *parseVisitor) VisitVars(c *parser.VarListDeclContext) *CompileError {
Account: *accAddr,
Asset: *assAddr,
})
p.readLockAccounts[*accAddr] = struct{}{}
if err != nil {
return LogicError(c, err)
}
Expand Down Expand Up @@ -674,12 +684,14 @@ func CompileFull(input string) CompileArtifacts {
}

visitor := parseVisitor{
errListener: errListener,
instructions: make([]byte, 0),
resources: make([]program.Resource, 0),
varIdx: make(map[string]machine.Address),
neededBalances: make(map[machine.Address]map[machine.Address]struct{}),
sources: map[machine.Address]struct{}{},
errListener: errListener,
instructions: make([]byte, 0),
resources: make([]program.Resource, 0),
varIdx: make(map[string]machine.Address),
neededBalances: make(map[machine.Address]map[machine.Address]struct{}),
sources: map[machine.Address]struct{}{},
writeLockAccounts: map[machine.Address]struct{}{},
readLockAccounts: map[machine.Address]struct{}{},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here the diff is weird because of golang's formatting, but the actual diff is this:

+ writeLockAccounts: map[machine.Address]struct{}{},
+ readLockAccounts:  map[machine.Address]struct{}{},
}

}

err := visitor.VisitScript(tree)
Expand All @@ -688,17 +700,24 @@ func CompileFull(input string) CompileArtifacts {
return artifacts
}

sources := make(machine.Addresses, 0)
for address := range visitor.sources {
sources = append(sources, address)
readLockAccounts := make(machine.Addresses, 0)
for address := range visitor.readLockAccounts {
readLockAccounts = append(readLockAccounts, address)
}
sort.Stable(readLockAccounts)

writeLockAccounts := make(machine.Addresses, 0)
for address := range visitor.writeLockAccounts {
writeLockAccounts = append(writeLockAccounts, address)
}
sort.Stable(sources)
sort.Stable(writeLockAccounts)

artifacts.Program = &program.Program{
Instructions: visitor.instructions,
Resources: visitor.resources,
NeededBalances: visitor.neededBalances,
Sources: sources,
Instructions: visitor.instructions,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

actual diff:

Instructions:   visitor.instructions,
Resources:      visitor.resources,
NeededBalances: visitor.neededBalances,
- Sources:        sources,
+ReadLockAccounts:  readLockAccounts,
+WriteLockAccounts: writeLockAccounts,

Resources: visitor.resources,
NeededBalances: visitor.neededBalances,
ReadLockAccounts: readLockAccounts,
WriteLockAccounts: writeLockAccounts,
Comment on lines +703 to +720
Copy link
Contributor

Choose a reason for hiding this comment

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

New logic for handling ReadLockAccounts and WriteLockAccounts

The addition of logic to process readLockAccounts and writeLockAccounts in the CompileFull function ensures that these accounts are properly sorted and included in the final Program structure. This change is crucial for implementing the new locking strategy.

Consider extracting the logic for processing readLockAccounts and writeLockAccounts into separate helper functions to improve code readability and maintainability. For example:

func processLockAccounts(accounts map[machine.Address]struct{}) machine.Addresses {
    result := make(machine.Addresses, 0, len(accounts))
    for address := range accounts {
        result = append(result, address)
    }
    sort.Stable(result)
    return result
}

// Usage in CompileFull:
readLockAccounts := processLockAccounts(visitor.readLockAccounts)
writeLockAccounts := processLockAccounts(visitor.writeLockAccounts)

This refactoring would reduce code duplication and make the CompileFull function more concise.

}

return artifacts
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1045,7 +1045,7 @@ func TestSetAccountMeta(t *testing.T) {
program2.OP_ASSET,
program2.OP_APUSH, 04, 00,
program2.OP_MONETARY_NEW,
program2.OP_TAKE_ALL,
program2.OP_TAKE_ALWAYS,
program2.OP_APUSH, 02, 00,
program2.OP_TAKE_MAX,
program2.OP_APUSH, 05, 00,
Expand Down Expand Up @@ -1177,7 +1177,7 @@ func TestVariableBalance(t *testing.T) {
program2.OP_ASSET,
program2.OP_APUSH, 04, 00,
program2.OP_MONETARY_NEW,
program2.OP_TAKE_ALL,
program2.OP_TAKE_ALWAYS,
program2.OP_APUSH, 02, 00,
program2.OP_TAKE_MAX,
program2.OP_APUSH, 05, 00,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ func (p *parseVisitor) VisitDestinationRecursive(c parser.IDestinationContext) *
case *parser.DestAccountContext:
p.AppendInstruction(program.OP_FUNDING_SUM)
p.AppendInstruction(program.OP_TAKE)
ty, _, err := p.VisitExpr(c.Expression(), true)
ty, destAddr, err := p.VisitExpr(c.Expression(), true)
if err != nil {
return err
}
Expand All @@ -32,6 +32,9 @@ func (p *parseVisitor) VisitDestinationRecursive(c parser.IDestinationContext) *
errors.New("wrong type: expected account as destination"),
)
}
if !p.isWorld(*destAddr) {
p.readLockAccounts[*destAddr] = struct{}{}
}
p.AppendInstruction(program.OP_SEND)
return nil
case *parser.DestInOrderContext:
Expand Down
35 changes: 32 additions & 3 deletions components/ledger/internal/machine/script/compiler/source.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,25 @@ func (p *parseVisitor) TakeFromSource(fallback *FallbackAccount) error {
return nil
}

func (p parseVisitor) isOverdraftUnbounded(overdraftCtx parser.ISourceAccountOverdraftContext) bool {
if overdraftCtx == nil {
return false
}

switch overdraftCtx.(type) {
case *parser.SrcAccountOverdraftUnboundedContext:
return true
case *parser.SrcAccountOverdraftSpecificContext:
return false

default:
// even though this branch should be unreachable,
// we default to `false` instead of panicking
// in order to have a more conservative behaviour
return false
}
}

// VisitSource returns the resource addresses of all the accounts,
// the addresses of accounts already emptied,
// and possibly a fallback account if the source has an unbounded overdraft allowance or contains @world
Expand Down Expand Up @@ -129,7 +148,11 @@ func (p *parseVisitor) VisitSource(c parser.ISourceContext, pushAsset func(), is
return nil, nil, nil, LogicError(c, err)
}
p.AppendInstruction(program.OP_MONETARY_NEW)
p.AppendInstruction(program.OP_TAKE_ALL)
if p.isWorld(*accAddr) {
p.AppendInstruction(program.OP_TAKE_ALWAYS)
} else {
p.AppendInstruction(program.OP_TAKE_ALL)
}
} else {
if p.isWorld(*accAddr) {
return nil, nil, nil, LogicError(c, errors.New("@world is already set to an unbounded overdraft"))
Expand All @@ -151,12 +174,18 @@ func (p *parseVisitor) VisitSource(c parser.ISourceContext, pushAsset func(), is
return nil, nil, nil, LogicError(c, err)
}
p.AppendInstruction(program.OP_MONETARY_NEW)
p.AppendInstruction(program.OP_TAKE_ALL)
p.AppendInstruction(program.OP_TAKE_ALWAYS)
f := FallbackAccount(*accAddr)
fallback = &f
}
}
neededAccounts[*accAddr] = struct{}{}

isUnboundedOverdraft := p.isWorld(*accAddr) || p.isOverdraftUnbounded(overdraft)
if !isUnboundedOverdraft {
p.writeLockAccounts[*accAddr] = struct{}{}
neededAccounts[*accAddr] = struct{}{}
}

emptiedAccounts[*accAddr] = struct{}{}

if fallback != nil && isAll {
Expand Down
49 changes: 32 additions & 17 deletions components/ledger/internal/machine/vm/machine.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"encoding/binary"
"fmt"
"math/big"
"slices"

"github.com/formancehq/ledger/internal/machine"

Expand Down Expand Up @@ -139,16 +140,16 @@ func (m *Machine) withdrawAlways(account machine.AccountAddress, mon machine.Mon
if accBalance, ok := m.Balances[account]; ok {
if balance, ok := accBalance[mon.Asset]; ok {
accBalance[mon.Asset] = balance.Sub(mon.Amount)
return &machine.Funding{
Asset: mon.Asset,
Parts: []machine.FundingPart{{
Account: account,
Amount: mon.Amount,
}},
}, nil
}
}
return nil, fmt.Errorf("missing %v balance from %v", mon.Asset, account)

return &machine.Funding{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of emitting the error when the account/asset balance is not found, return the required needed amount.
This is what allows unbounded accounts to send without locking their balance

Asset: mon.Asset,
Parts: []machine.FundingPart{{
Account: account,
Amount: mon.Amount,
}},
}, nil
}

func (m *Machine) credit(account machine.AccountAddress, funding machine.Funding) {
Expand All @@ -170,8 +171,16 @@ func (m *Machine) repay(funding machine.Funding) {
if part.Account == "world" {
continue
}
balance := m.Balances[part.Account][funding.Asset]
m.Balances[part.Account][funding.Asset] = balance.Add(part.Amount)
accountBalance, ok := m.Balances[part.Account]
if !ok {
// no asset: the source has to be an unbounded source
// which NEVER appears as bounded
// this means we don't need to track it's balance
continue
}

balance := accountBalance[funding.Asset]
accountBalance[funding.Asset] = balance.Add(part.Amount)
}
}

Expand Down Expand Up @@ -574,6 +583,9 @@ func (m *Machine) ResolveResources(ctx context.Context, store Store) ([]string,
if err != nil {
return nil, nil, err
}
if val.GetType() == machine.TypeAccount {
involvedAccountsMap[machine.Address(idx)] = string(val.(machine.AccountAddress))
}
case program.VariableAccountBalance:
acc, _ := m.getResource(res.Account)
address := string((*acc).(machine.AccountAddress))
Expand Down Expand Up @@ -607,16 +619,19 @@ func (m *Machine) ResolveResources(ctx context.Context, store Store) ([]string,
m.Resources = append(m.Resources, val)
}

involvedAccounts := make([]string, 0)
involvedSources := make([]string, 0)
for _, accountAddress := range involvedAccountsMap {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

⚠️ IMPORTANT DIFF
Instead of marking every possible account that appears within the program (directly or indirectly - e.g. the result of meta()) as readLock account, we only iterate through m.Program.ReadLockAccounts

involvedAccounts = append(involvedAccounts, accountAddress)
readLockAccounts := make([]string, 0)
for _, accountAddress := range m.Program.ReadLockAccounts {
readLockAccounts = append(readLockAccounts, involvedAccountsMap[accountAddress])
}
for _, machineAddress := range m.Program.Sources {
involvedSources = append(involvedSources, involvedAccountsMap[machineAddress])

writeLockAccounts := make([]string, 0)
for _, machineAddress := range m.Program.WriteLockAccounts {
writeLockAccounts = append(writeLockAccounts, involvedAccountsMap[machineAddress])
}

return involvedAccounts, involvedSources, nil
slices.Sort(readLockAccounts)
slices.Sort(writeLockAccounts)
return readLockAccounts, writeLockAccounts, nil
}

func (m *Machine) SetVarsFromJSON(vars map[string]string) error {
Expand Down
Loading