Add ledger.GetBlockAddresses()

[tolikzinovyev]

Summary

Indexer needs to know all accounts referenced in a block to be able to read all of them in one batch. This PR creates a function that returns all such account addresses.

[codecov-commenter]

Codecov Report

Merging #2872 (3831123) into master (83fb239) will decrease coverage by 0.03%.
The diff coverage is 62.50%.

@@            Coverage Diff             @@
##           master    #2872      +/-   ##
==========================================
- Coverage   47.37%   47.33%   -0.04%     
==========================================
  Files         351      351              
  Lines       56523    56529       +6     
==========================================
- Hits        26776    26760      -16     
- Misses      26743    26761      +18     
- Partials     3004     3008       +4     

Impacted Files	Coverage Δ
ledger/eval.go	76.17% <62.50%> (-0.66%)	⬇️
ledger/roundlru.go	90.56% <0.00%> (-5.67%)	⬇️
ledger/blockqueue.go	82.18% <0.00%> (-2.88%)	⬇️
network/requestTracker.go	70.25% <0.00%> (-1.30%)	⬇️
catchup/peerSelector.go	98.95% <0.00%> (-1.05%)	⬇️
cmd/tealdbg/debugger.go	72.86% <0.00%> (-1.01%)	⬇️
ledger/acctupdates.go	62.13% <0.00%> (-0.51%)	⬇️
network/wsNetwork.go	60.90% <0.00%> (-0.19%)	⬇️
catchup/service.go	69.35% <0.00%> (ø)
network/wsPeer.go	75.20% <0.00%> (+0.55%)	⬆️
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83fb239...3831123. Read the comment docs.

[tsachiherman]

The change looks good - but they don't perform that great. I tested your changes using

go test -v . --run XX --bench BenchmarkBlockEvaluatorRAMNoCrypto -count=10 | grep BenchmarkBlockEvaluatorRAMNoCrypto

and received the following performance metrics on master:

BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 945332700 ns/op	     18907 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 887004810 ns/op	     17741 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 898731697 ns/op	     17976 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 905146646 ns/op	     18104 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 895749462 ns/op	     17916 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 876699177 ns/op	     17535 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 889700168 ns/op	     17795 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 888446000 ns/op	     17770 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 888201852 ns/op	     17765 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 916630812 ns/op	     18333 ns/eval_validate_tx

compared to the ones from your branch:

BenchmarkBlockEvaluatorRAMNoCrypto-8   	       1	1016715840 ns/op	     20335 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       1	1026562100 ns/op	     20533 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	1005568624 ns/op	     20112 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       1	1008136765 ns/op	     20164 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       1	1016944071 ns/op	     20340 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	1016153946 ns/op	     20324 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 974119522 ns/op	     19483 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 957210924 ns/op	     19145 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 879668276 ns/op	     17594 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 913933502 ns/op	     18279 ns/eval_validate_tx

In the vast of the cases, master perform better compared to your branch. I would suggest that you'll keep the existing implementation of loadAccounts as is, and just have your own changes ( you might want to add some comments that it's a code duplication that need to be converged ).

For the purposes of the indexer, we can optimize that code on another task - but there is no reason to introduce a regression to the master.

[winder]

Would it make sense to add this as a function to SignedTxnInBlock instead of as part of the evaluator?

[tsachiherman]

Would it make sense to add this as a function to SignedTxnInBlock instead of as part of the evaluator?

I would be ok if we were to add the method to the transactions package. But why SignedTxnInBlock and not Transaction ?

[winder]

Would it make sense to add this as a function to SignedTxnInBlock instead of as part of the evaluator?

I would be ok if we were to add the method to the transactions package. But why SignedTxnInBlock and not Transaction ?

Transaction would be better. I was thinking we needed the ApplyData in order to figure out who participates in the inner transactions, but txn.ApplicationCallTxnFields.Accounts is available for that.

Do we always need all of these accounts available? I suppose the TEAL evaluation needs to read accounts regardless of whether they are modified...

If all of these accounts are added to the txn participation table, this is going to change the result of a txns by account lookup. We may want to add this to the release notes, and possibly add a new address role for it.

[tolikzinovyev]

I can't reproduce the difference in performance (perhaps because I'm using the newest version of go). I changed the code slightly. Could you run the benchmark again?

[tsachiherman]

I can't reproduce the difference in performance (perhaps because I'm using the newest version of go). I changed the code slightly. Could you run the benchmark again?

I'm using

go version go1.16.7 darwin/amd64

and running the same benchmark again:

BenchmarkBlockEvaluatorRAMNoCrypto
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 937884934 ns/op	     18758 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	1104762888 ns/op	     22097 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       1	1051855114 ns/op	     21039 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 945192180 ns/op	     18905 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 964409754 ns/op	     19289 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 896160418 ns/op	     17924 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 945274962 ns/op	     18906 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 971673884 ns/op	     19435 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       1	1020561048 ns/op	     20412 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 971733400 ns/op	     19436 ns/eval_validate_tx

is slightly better, but not there yet.

[tolikzinovyev]

It might be something to do with the new optimization in go 1.17. https://golang.org/doc/go1.17#compiler

How significant is this benchmark anyway? The cost of eval should be dominated by crypto and disk.

[tsachiherman]

There is no reason to have any performance loss for adding new, non-interfering functionality. I'm sure that the performance loss would have been much more noticeable if we had a benchmark for loadAccounts - but we don't.

Given that refactoring this code isn't really what you're after, just rollback the changes in loadAccounts and it will be good to go. Once we move to go 1.17, I'll be more than happy to re-test that.

[tsachiherman]

I think that you meant

*out = append(*out, txn.ApplicationCallTxnFields.Accounts...)

[tolikzinovyev]

thanks!

[tsachiherman]

looks good to me.