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

txngroup-deltas: Fix pointer bug copying deltas for txngroups #5375

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion daemon/algod/api/server/v2/test/handlers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2077,7 +2077,7 @@ func TestDeltasForTxnGroup(t *testing.T) {
blk1 := bookkeeping.BlockHeader{Round: 1}
blk2 := bookkeeping.BlockHeader{Round: 2}
delta1 := ledgercore.StateDelta{Hdr: &blk1}
delta2 := ledgercore.StateDelta{Hdr: &blk2}
delta2 := ledgercore.StateDelta{Hdr: &blk2, KvMods: map[string]ledgercore.KvValueDelta{"bx1": {Data: []byte("foobar")}}}
txn1 := transactions.SignedTxnWithAD{SignedTxn: transactions.SignedTxn{Txn: transactions.Transaction{Type: protocol.PaymentTx}}}
groupID1, err := crypto.DigestFromString(crypto.Hash([]byte("hello")).String())
require.NoError(t, err)
Expand Down Expand Up @@ -2160,6 +2160,7 @@ func TestDeltasForTxnGroup(t *testing.T) {
require.NoError(t, err)
err = json.Unmarshal(rec.Body.Bytes(), &groupResponse)
require.NoError(t, err)
require.NotNil(t, groupResponse["KvMods"])
groupHdr, ok = groupResponse["Hdr"].(map[string]interface{})
require.True(t, ok)
require.Equal(t, delta2.Hdr.Round, basics.Round(groupHdr["rnd"].(float64)))
Expand Down
6 changes: 5 additions & 1 deletion ledger/eval/eval_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,7 @@ type evalTestLedger struct {
rewardsPool basics.Address
latestTotals ledgercore.AccountTotals
tracer logic.EvalTracer
boxes map[string][]byte
Copy link
Contributor

Choose a reason for hiding this comment

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

Every test that uses this evalTestLedger creeps me out. It is a mock that maybe has the same behaviour as a real ledger, and maybe not. So every time we extend it to try to do more, it feels even more as though every test should be converted to use a real ledger. DoubleLedger and TestConsensusRange have some real benefits in terms of accuracy and testing across changes in old versions.

I'm not going to hold up this PR, but we have to wean ourselves off this thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree the actual implementation of the test ledger is pretty far from the actual ledger. It does seem difficult to mock since the data retrieved in the actual ledger hits the caches (trackers) first and then disk if the cache misses.

I haven't looked into the DoubleLedger or TestConsensusRange, but I will do next time I need to test things requiring the ledger.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using real ledger will make all dependent tests very slow. I guess we need just one unified mock with some default implementation and let embedders to override methods they care about

}

// newTestLedger creates a in memory Ledger that is as realistic as
Expand All @@ -611,6 +612,7 @@ func newTestLedger(t testing.TB, balances bookkeeping.GenesisBalances) *evalTest
feeSink: balances.FeeSink,
rewardsPool: balances.RewardsPool,
tracer: nil,
boxes: make(map[string][]byte),
}

protoVersion := protocol.ConsensusFuture
Expand Down Expand Up @@ -728,7 +730,9 @@ func (ledger *evalTestLedger) LookupAsset(rnd basics.Round, addr basics.Address,
}

func (ledger *evalTestLedger) LookupKv(rnd basics.Round, key string) ([]byte, error) {
panic("unimplemented")
// The test ledger only has one view of the value of a box--no rnd based retrieval is implemented currently
val, _ := ledger.boxes[key]
return val, nil
Comment on lines +733 to +735
Copy link
Contributor

@jannotti jannotti May 11, 2023

Choose a reason for hiding this comment

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

This is the kind of craziness I'm talking about. Shouldn't we use a real ledger, and not constantly make little updates to approximate its behavior here?

}

// GenesisHash returns the genesis hash for this ledger.
Expand Down
69 changes: 54 additions & 15 deletions ledger/eval/txntracer.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,50 @@ type StateDeltaSubset struct {
}

func convertStateDelta(delta ledgercore.StateDelta) StateDeltaSubset {
// The StateDelta object returned through the EvalTracer has its values deleted between txn groups to avoid
// reallocation during evaluation.
// This means the map values need to be copied (to avoid deletion) since they are all passed by reference.
kvmods := make(map[string]ledgercore.KvValueDelta, len(delta.KvMods))
for k1, v1 := range delta.KvMods {
kvmods[k1] = v1
}
txids := make(map[transactions.Txid]ledgercore.IncludedTransactions, len(delta.Txids))
for k2, v2 := range delta.Txids {
txids[k2] = v2
}
txleases := make(map[ledgercore.Txlease]basics.Round, len(delta.Txleases))
for k3, v3 := range delta.Txleases {
txleases[k3] = v3
tzaffi marked this conversation as resolved.
Show resolved Hide resolved
}
creatables := make(map[basics.CreatableIndex]ledgercore.ModifiedCreatable, len(delta.Creatables))
for k4, v4 := range delta.Creatables {
creatables[k4] = v4
}
var accR []ledgercore.BalanceRecord
var appR []ledgercore.AppResourceRecord
var assetR []ledgercore.AssetResourceRecord
if len(delta.Accts.Accts) > 0 {
accR = make([]ledgercore.BalanceRecord, len(delta.Accts.Accts))
copy(accR, delta.Accts.Accts)
}
if len(delta.Accts.AppResources) > 0 {
appR = make([]ledgercore.AppResourceRecord, len(delta.Accts.AppResources))
copy(appR, delta.Accts.AppResources)
}
if len(delta.Accts.AssetResources) > 0 {
assetR = make([]ledgercore.AssetResourceRecord, len(delta.Accts.AssetResources))
copy(assetR, delta.Accts.AssetResources)
}
return StateDeltaSubset{
Accts: delta.Accts,
KvMods: delta.KvMods,
Txids: delta.Txids,
Txleases: delta.Txleases,
Creatables: delta.Creatables,
Accts: ledgercore.AccountDeltas{
Accts: accR,
AppResources: appR,
AssetResources: assetR,
},
KvMods: kvmods,
Txids: txids,
Txleases: txleases,
Creatables: creatables,
Hdr: delta.Hdr,
}
}
Expand All @@ -65,8 +103,8 @@ type TxnGroupDeltaTracer struct {
lookback uint64
// no-op methods we don't care about
logic.NullEvalTracer
// txnGroupDeltas stores the StateDelta objects for each round, indexed by all the IDs within the group
txnGroupDeltas map[basics.Round]map[crypto.Digest]*ledgercore.StateDelta
// txnGroupDeltas stores the StateDeltaSubset objects for each round, indexed by all the IDs within the group
txnGroupDeltas map[basics.Round]map[crypto.Digest]*StateDeltaSubset
// latestRound is the most recent round seen via the BeforeBlock hdr
latestRound basics.Round
}
Expand All @@ -75,7 +113,7 @@ type TxnGroupDeltaTracer struct {
func MakeTxnGroupDeltaTracer(lookback uint64) *TxnGroupDeltaTracer {
return &TxnGroupDeltaTracer{
lookback: lookback,
txnGroupDeltas: make(map[basics.Round]map[crypto.Digest]*ledgercore.StateDelta),
txnGroupDeltas: make(map[basics.Round]map[crypto.Digest]*StateDeltaSubset),
}
}

Expand All @@ -87,24 +125,25 @@ func (tracer *TxnGroupDeltaTracer) BeforeBlock(hdr *bookkeeping.BlockHeader) {
delete(tracer.txnGroupDeltas, hdr.Round-basics.Round(tracer.lookback))
tracer.latestRound = hdr.Round
// Initialize the delta map for the round
tracer.txnGroupDeltas[tracer.latestRound] = make(map[crypto.Digest]*ledgercore.StateDelta)
tracer.txnGroupDeltas[tracer.latestRound] = make(map[crypto.Digest]*StateDeltaSubset)
}

// AfterTxnGroup implements the EvalTracer interface for txn group boundaries
func (tracer *TxnGroupDeltaTracer) AfterTxnGroup(ep *logic.EvalParams, deltas *ledgercore.StateDelta, evalError error) {
if deltas == nil {
return
}
deltaSub := convertStateDelta(*deltas)
tracer.deltasLock.Lock()
defer tracer.deltasLock.Unlock()
txnDeltaMap := tracer.txnGroupDeltas[tracer.latestRound]
for _, txn := range ep.TxnGroup {
// Add Group ID
if !txn.Txn.Group.IsZero() {
txnDeltaMap[txn.Txn.Group] = deltas
txnDeltaMap[txn.Txn.Group] = &deltaSub
}
// Add Txn ID
txnDeltaMap[crypto.Digest(txn.ID())] = deltas
txnDeltaMap[crypto.Digest(txn.ID())] = &deltaSub
}
}

Expand All @@ -117,7 +156,7 @@ func (tracer *TxnGroupDeltaTracer) GetDeltasForRound(rnd basics.Round) ([]TxnGro
return nil, fmt.Errorf("round %d not found in txnGroupDeltaTracer", rnd)
}
// Dedupe entries in our map and collect Ids
var deltas = map[*ledgercore.StateDelta][]string{}
var deltas = map[*StateDeltaSubset][]string{}
for id, delta := range rndEntries {
if _, present := deltas[delta]; !present {
deltas[delta] = append(deltas[delta], id.String())
Expand All @@ -127,19 +166,19 @@ func (tracer *TxnGroupDeltaTracer) GetDeltasForRound(rnd basics.Round) ([]TxnGro
for delta, ids := range deltas {
deltasForRound = append(deltasForRound, TxnGroupDeltaWithIds{
Ids: ids,
Delta: convertStateDelta(*delta),
Delta: *delta,
})
}
return deltasForRound, nil
}

// GetDeltaForID retruns the StateDelta associated with the group of transaction executed for the supplied ID (txn or group)
// GetDeltaForID returns the StateDelta associated with the group of transaction executed for the supplied ID (txn or group)
func (tracer *TxnGroupDeltaTracer) GetDeltaForID(id crypto.Digest) (StateDeltaSubset, error) {
tracer.deltasLock.RLock()
defer tracer.deltasLock.RUnlock()
for _, deltasForRound := range tracer.txnGroupDeltas {
if delta, exists := deltasForRound[id]; exists {
return convertStateDelta(*delta), nil
return *delta, nil
algochoi marked this conversation as resolved.
Show resolved Hide resolved
}
}
return StateDeltaSubset{}, fmt.Errorf("unable to find delta for id: %s", id)
Expand Down
Loading