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

Make inner transaction creatable ID lookup more resilient. #777

Merged
merged 4 commits into from
Nov 9, 2021

Conversation

winder
Copy link
Contributor

@winder winder commented Nov 8, 2021

Summary

Make the inner transaction ID lookup more resilient by passing in a block pointer even though it shouldn't be required.

A crash was reported when using sandbox with stable algod (3.0.1) and develop indexer (Nov 8):

Starting indexer against algod.
Connecting to algod:4001 (172.18.0.2:4001)
saving to 'genesis.json'
genesis.json         100% |********************************|  1822  0:00:00 ETA
'genesis.json' saved
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x158 pc=0x1284f16]

goroutine 22 [running]:
github.com/algorand/indexer/idb/postgres/internal/writer.transactionAssetID(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/opt/indexer/idb/postgres/internal/writer/writer.go:167 +0x56
github.com/algorand/indexer/idb/postgres/internal/writer.(*Writer).addInnerTransactions(0xc00048e728, 0xc00048d418, 0xc0000ba780, 0x1, 0x0, 0xc0004b0280, 0x34, 0xc00013c8c0, 0x1, 0x1, ...)
	/opt/indexer/idb/postgres/internal/writer/writer.go:189 +0x183
github.com/algorand/indexer/idb/postgres/internal/writer.(*Writer).addTransactions(0xc00048e728, 0xc0000ba780, 0xc000294000, 0x2, 0x2, 0xc65f57e557c98cf5, 0x8cc1f3760e6e675a)
	/opt/indexer/idb/postgres/internal/writer/writer.go:251 +0x956
github.com/algorand/indexer/idb/postgres/internal/writer.(*Writer).AddBlock(0xc00048e728, 0xc0000ba780, 0xc000294000, 0x2, 0x2, 0xc0002a8000, 0x5, 0x8, 0xc000319770, 0xc0003197a0, ...)
	/opt/indexer/idb/postgres/internal/writer/writer.go:507 +0x77
github.com/algorand/indexer/idb/postgres.(*IndexerDb).AddBlock.func1(0x1a8bfc0, 0xc00000e120, 0x0, 0x0)
	/opt/indexer/idb/postgres/postgres.go:255 +0xace
github.com/algorand/indexer/idb/postgres/internal/util.attemptTx(0x1a8bfc0, 0xc00000e120, 0xc0000135d0, 0x0, 0x0)
	/opt/indexer/idb/postgres/internal/util/util.go:25 +0xb6
github.com/algorand/indexer/idb/postgres/internal/util.TxWithRetry(0xc0004485b0, 0x16e1632, 0xc, 0x0, 0x0, 0x0, 0x0, 0xc00048f5d0, 0xc0004484d0, 0x0, ...)
	/opt/indexer/idb/postgres/internal/util/util.go:52 +0xbe
github.com/algorand/indexer/idb/postgres.(*IndexerDb).txWithRetry(...)
	/opt/indexer/idb/postgres/postgres.go:115
github.com/algorand/indexer/idb/postgres.(*IndexerDb).AddBlock(0xc0002cfd40, 0xc0000ba780, 0x0, 0x0)
	/opt/indexer/idb/postgres/postgres.go:263 +0x204
github.com/algorand/indexer/importer.(*Importer).ImportBlock(0xc00011a120, 0xc0000ba780, 0x2480de0, 0x0)
	/opt/indexer/importer/importer.go:25 +0x77
main.(*blockImporterHandler).HandleBlock(0xc00011a120, 0xc0000ba780)
	/opt/indexer/cmd/algorand-indexer/daemon.go:168 +0x77
github.com/algorand/indexer/fetcher.(*fetcherImpl).handleBlock(0xc00036ba20, 0xc0000ba780)
	/opt/indexer/fetcher/fetcher.go:237 +0x6c
github.com/algorand/indexer/fetcher.(*fetcherImpl).processQueue(0xc00036ba20)
	/opt/indexer/fetcher/fetcher.go:107 +0x32
created by github.com/algorand/indexer/fetcher.(*fetcherImpl).Run
	/opt/indexer/fetcher/fetcher.go:195 +0xc8

@winder winder requested a review from tolikzinovyev November 8, 2021 13:49
@winder winder self-assigned this Nov 8, 2021
@winder winder changed the title Prevent crash with non-standard configuration. Make inner transaction creatable ID lookup more resilient. Nov 8, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 9, 2021

Codecov Report

Merging #777 (97cb5b4) into develop (d2a28b6) will increase coverage by 0.08%.
The diff coverage is 72.22%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #777      +/-   ##
===========================================
+ Coverage    54.47%   54.55%   +0.08%     
===========================================
  Files           28       28              
  Lines         3870     3877       +7     
===========================================
+ Hits          2108     2115       +7     
  Misses        1481     1481              
  Partials       281      281              
Impacted Files Coverage Δ
idb/postgres/internal/writer/writer.go 77.77% <72.22%> (+0.71%) ⬆️

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 d2a28b6...97cb5b4. Read the comment docs.

idb/postgres/internal/writer/writer.go Outdated Show resolved Hide resolved
idb/postgres/internal/writer/writer.go Outdated Show resolved Hide resolved
idb/postgres/internal/writer/writer_test.go Outdated Show resolved Hide resolved
// pre v30 transactions do not have ApplyData.ConfigAsset or InnerTxns
// so txn counter + payset pos calculation is OK
assetid = block.TxnCounter - uint64(len(block.Payset)) + uint64(intra) + 1
}
case protocol.AssetConfigTx:
assetid = uint64(txn.ConfigAsset)
assetid = uint64(txn.Txn.ConfigAsset)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to add a test that txn.Txn.ConfigAsset is actually used to set the asset column. The reason why we have this bug is because we didn't have a test like this. :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added one test that fails until making this change. It doesn't check ApplicationID, is there anything else it's missing?

@winder winder merged commit 2f003fb into develop Nov 9, 2021
@winder winder deleted the will/stop-crash branch November 9, 2021 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants