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

Eric-Warehime · 2023-05-10T18:50:46Z

Summary

The KvMods (and other fields in the StateDelta which are maps) have their values deleted when the roundCoW gets recycled. Because we're not copying this data out of the delta in the txn tracer, this affects the results returned by the tracer.

I've now copied all of the fields out of the maps in the StateDelta (for those that we care about returning).

Test Plan

Adding full unit tests for kvmods--other data should function similarly. This required adding some functionality to the eval test ledger which was not initially implemented during the Boxes PR.

codecov · 2023-05-10T19:24:12Z

Codecov Report

Merging #5375 (4791b5f) into master (b988971) will increase coverage by 0.09%.
The diff coverage is 92.68%.

@@            Coverage Diff             @@
##           master    #5375      +/-   ##
==========================================
+ Coverage   55.25%   55.35%   +0.09%     
==========================================
  Files         452      452              
  Lines       63614    63643      +29     
==========================================
+ Hits        35153    35229      +76     
+ Misses      26062    26004      -58     
- Partials     2399     2410      +11

Impacted Files	Coverage Δ
ledger/eval/txntracer.go	`92.59% <92.68%> (-1.64%)`	⬇️

... and 20 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

algochoi

Nice catch, adding the box in the txntracer_test seems to show that the map is being overriden. I had a question in PR comment below

ledger/eval/txntracer.go

tzaffi

Looks good. One thought crosses my mind. The current box app creates the box. I'm wondering if there could be a benefit to testing some of the following - or these may be overkill:

modify a box with box_replace
delete a box with box_del
compose a box_create with a box_del in the same app call and/or in subsequent calls of the same group, or subsequent inner app calls.
similarly, composing box_replace's

Eric-Warehime · 2023-05-11T15:30:02Z

Looks good. One thought crosses my mind. The current box app creates the box. I'm wondering if there could be a benefit to testing some of the following - or these may be overkill:

modify a box with box_replace

delete a box with box_del

compose a box_create with a box_del in the same app call and/or in subsequent calls of the same group, or subsequent inner app calls.

similarly, composing box_replace's

I can add a bit more, but in general none of this code cares what the TEAL code is doing, just that it properly conveys the deltas associated with it. And the fact that KvMods are present pretty much means all KvMod opcode deltas will be present as well.

The inner txn box mods would probably be worthwhile to test. Potentially having subsequent updates to the same box as well I guess?

tzaffi · 2023-05-11T15:36:46Z

I can add a bit more, but in general none of this code cares what the TEAL code is doing, just that it properly conveys the deltas associated with it. And the fact that KvMods are present pretty much means all KvMod opcode deltas will be present as well.

Totally up to you. If you're viewing the KvMods as a "black box" in which case the current scenario proves that the PR reveals the black box as is, then no further tests are required. But if the logic could be tricked into believing that there aren't any txndelta's when actually there were (e.g. by piling on changes, or reversing them) then another test case would be called for.

The inner txn box mods would probably be worthwhile to test. Potentially having subsequent updates to the same box as well I guess?

Yeah, in the past, the inner txns seemed to always be the most error prone due to recursion.

algochoi

LGTM, thanks for addressing comments!

Eric-Warehime · 2023-05-11T16:43:04Z

@tzaffi Yep, for the most part I'd like to treat it as an opaque operation, but that operation was only intended to be correct at the block level and not necessarily at the txn-group level so it's worthwhile to add more tests. I've added a box_del call in an inner txn to the test.

Let me know if you feel like we really need to test anything else that's critical path w/r/t the deltas here.

tzaffi · 2023-05-11T16:51:55Z

ledger/eval/txntracer_test.go

+				"bx:\x00\x00\x00\x00\x00\x00\x03\xefgoodbyebox": {
+					OldData: nil,
+					Data:    nil,
+				},


So this ghost of boxes past is picked up because the box was created then deleted?

Yes, nil for the Data field represents a deleted box.

It is weird that the OldData field is 0 bytes for hellobox but nil for goodbyebox even though both of those were created in the group. It could just be a matter of how I've implemented LookupKv in the test ledger though so the actual ledger might not act in the same way.

tzaffi

LGTM

jannotti

I'm willing to merge this, let me know if you want to do anything about the things I mentioned.

@jasonpaulos should also see it though - it feels like you're both addressing some copying issues right now.

jannotti · 2023-05-11T18:51:14Z

ledger/eval/txntracer_test.go

+								ApprovalProgram:   []byte{0x08, 0x80, 0x08, 0x68, 0x65, 0x6c, 0x6c, 0x6f, 0x62, 0x6f, 0x78, 0x81, 0x0a, 0xb9, 0x48, 0x81, 0x01},
+								ClearStateProgram: []byte{0x08, 0x81, 0x01},


I guess you didn't put these here originally, but the fact that we're editing hex bytecode here seems bad. I assume this is a compiled program, can we use logic.AssembleString so it makes more sense?

Updated. I actually did write those originally, just not in this PR. I wasn't aware that this was the proper way to encode these.

jannotti · 2023-05-11T18:54:12Z

ledger/eval/eval_test.go

@@ -600,6 +600,7 @@ type evalTestLedger struct {
 	rewardsPool         basics.Address
 	latestTotals        ledgercore.AccountTotals
 	tracer              logic.EvalTracer
+	boxes               map[string][]byte


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.

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.

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

jannotti · 2023-05-11T18:54:53Z

ledger/eval/eval_test.go

+	// 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


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?

ledger/eval/txntracer.go

algorandskiy · 2023-05-11T21:50:21Z

ledger/eval/eval_test.go

@@ -600,6 +600,7 @@ type evalTestLedger struct {
 	rewardsPool         basics.Address
 	latestTotals        ledgercore.AccountTotals
 	tracer              logic.EvalTracer
+	boxes               map[string][]byte


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

algorandskiy · 2023-05-11T22:00:17Z

ledger/eval/txntracer_test.go

@@ -223,12 +311,8 @@ int 1`,
 						Addr: addrs[0],
 						Params: ledgercore.AppParamsDelta{
 							Params: &basics.AppParams{
-								ApprovalProgram: []byte{0x06, 0x80, 0x01, 0x61, 0xb0, 0xb1, 0x81, 0x06, 0xb2, 0x10, 0x81, 0x00, 0xb2, 0x19, 0x80, 0x07,


nooo... we like reading bytecode

Fix pointer bug copying deltas for txngroups

f18e8cc

Eric-Warehime added the Bug-Fix label May 10, 2023

Fix lint

6b5d8dc

Eric-Warehime requested review from cce, winder, tzaffi, algorandskiy, shiqizng and algochoi May 10, 2023 19:27

Eric-Warehime marked this pull request as ready for review May 10, 2023 19:27

algochoi reviewed May 10, 2023

View reviewed changes

ledger/eval/txntracer.go Outdated Show resolved Hide resolved

ledger/eval/txntracer.go Show resolved Hide resolved

ledger/eval/txntracer.go Outdated Show resolved Hide resolved

tzaffi reviewed May 10, 2023

View reviewed changes

ledger/eval/txntracer.go Show resolved Hide resolved

Eric-Warehime mentioned this pull request May 10, 2023

txngroup-deltas: Add group delta tests algorand/algorand-sdk-testing#281

Merged

PR comments

66cbb88

tzaffi reviewed May 10, 2023

View reviewed changes

algochoi previously approved these changes May 11, 2023

View reviewed changes

Add box_del to txntracer tests

f2f0e92

Eric-Warehime dismissed algochoi’s stale review via f2f0e92 May 11, 2023 16:40

Eric-Warehime requested a review from tzaffi May 11, 2023 16:43

tzaffi reviewed May 11, 2023

View reviewed changes

tzaffi previously approved these changes May 11, 2023

View reviewed changes

algochoi previously approved these changes May 11, 2023

View reviewed changes

jannotti previously approved these changes May 11, 2023

View reviewed changes

Use logic.AssembleString in txntracer test

b2c731d

Eric-Warehime dismissed stale reviews from jannotti and algochoi via b2c731d May 11, 2023 19:17

Eric-Warehime dismissed tzaffi’s stale review via b2c731d May 11, 2023 19:17

jasonpaulos reviewed May 11, 2023

View reviewed changes

ledger/eval/txntracer.go Outdated Show resolved Hide resolved

Eric-Warehime added 2 commits May 11, 2023 12:59

Add second txn group to txntracer test

4e8215a

Copy contents of Accs

bdf35d8

Eric-Warehime requested review from jasonpaulos, jannotti, algochoi and tzaffi May 11, 2023 20:44

jasonpaulos previously approved these changes May 11, 2023

View reviewed changes

ledger/eval/txntracer.go Outdated Show resolved Hide resolved

algorandskiy approved these changes May 11, 2023

View reviewed changes

algorandskiy previously approved these changes May 11, 2023

View reviewed changes

Init maps with proper size

4791b5f

Eric-Warehime dismissed stale reviews from algorandskiy and jasonpaulos via 4791b5f May 11, 2023 22:05

Eric-Warehime requested review from jasonpaulos and algorandskiy May 11, 2023 22:35

jasonpaulos approved these changes May 11, 2023

View reviewed changes

algorandskiy approved these changes May 11, 2023

View reviewed changes

algorandskiy merged commit d7c5e54 into algorand:master May 11, 2023
24 checks passed

Algo-devops-service mentioned this pull request May 12, 2023

go-algorand 3.16.0-beta Release PR #5382

Closed

Algo-devops-service mentioned this pull request May 22, 2023

go-algorand 3.16.0-beta Release PR #5406

Merged

This was referenced May 24, 2023

go-algorand 3.16.0-beta Release PR #5417

Merged

go-algorand 3.16.0-beta Release PR #5430

Merged

go-algorand 3.16.0-beta Release PR #5434

Merged

Algo-devops-service mentioned this pull request Jun 12, 2023

go-algorand 3.16.1-stable Release PR #5465

Merged

onetechnical mentioned this pull request Jun 14, 2023

go-algorand 3.16.2-stable Release PR #5469

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Eric-Warehime commented May 10, 2023

codecov bot commented May 10, 2023 •

edited

algochoi left a comment

tzaffi left a comment •

edited

Eric-Warehime commented May 11, 2023

tzaffi commented May 11, 2023

algochoi left a comment

Eric-Warehime commented May 11, 2023

tzaffi May 11, 2023 •

edited

Eric-Warehime May 11, 2023

tzaffi left a comment

jannotti left a comment

jannotti May 11, 2023

Eric-Warehime May 11, 2023

jannotti May 11, 2023

Eric-Warehime May 11, 2023

algorandskiy May 11, 2023

jannotti May 11, 2023 •

edited

algorandskiy May 11, 2023

algorandskiy May 11, 2023

		ApprovalProgram: []byte{0x08, 0x80, 0x08, 0x68, 0x65, 0x6c, 0x6c, 0x6f, 0x62, 0x6f, 0x78, 0x81, 0x0a, 0xb9, 0x48, 0x81, 0x01},
		ClearStateProgram: []byte{0x08, 0x81, 0x01},

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

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

Conversation

Eric-Warehime commented May 10, 2023

Summary

Test Plan

codecov bot commented May 10, 2023 • edited

Codecov Report

algochoi left a comment

Choose a reason for hiding this comment

tzaffi left a comment • edited

Choose a reason for hiding this comment

Eric-Warehime commented May 11, 2023

tzaffi commented May 11, 2023

algochoi left a comment

Choose a reason for hiding this comment

Eric-Warehime commented May 11, 2023

tzaffi May 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzaffi left a comment

Choose a reason for hiding this comment

jannotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannotti May 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 10, 2023 •

edited

tzaffi left a comment •

edited

tzaffi May 11, 2023 •

edited

jannotti May 11, 2023 •

edited