Add Tx Indexer Extension #1143

aaronbuchwald · 2024-07-15T15:53:46Z

This PR refactors the tx indexing from the MorpheusVM/TokenVM controller code into a separate module under extensions/indexer

This also fixes an apparent bug where the database created internally to the MorpheusVM/TokenVM for tx indexing would never be closed.

joshua-kim · 2024-07-16T14:43:02Z

examples/morpheusvm/controller/action_indexer.go

+
+var _ indexer.SuccessfulTxIndexer = &successfulTxIndexer{}
+
+type successfulTxIndexer struct {


IMO these implementing the indexer interface (tokenvm one as well) seems weird to me because this is not an indexer. I'm wondering if this metrics code should go back to living in the Controller code.

We could rename it as well. We could more accurately call it an AcceptedSubscriber.

This would be more accurate to exactly what it's doing and an indexer would be one thing you could build as an AcceptedSubscriber. Similarly, you could pass in an acceptedActionHandler or similar, which would be just this for the MorpheusVM and would also maintain the in-memory order book in the case of TokenVM.

extensions/indexer/indexer.go

joshua-kim · 2024-07-16T14:48:23Z

examples/morpheusvm/controller/controller.go

 	if err != nil {
 		return vm.Config{}, nil, nil, nil, nil, nil, nil, nil, err
 	}
+	c.txIndexer = indexer.NewTxIndexer(


I would prefer if we just had a no-op implementation of the indexer interface that was used instead if GetStoreTransactions was set here

This is just keeping the same pattern that was there before. The for loop currently does two things, it iterates and indexes every transaction and then for each successful transaction it performs some action for each of the multi-actions that were included.

Alternatively, if we switch to AcceptedSubscriber, then the accepted action indexer can just be completely separate.

Yeah I would prefer it be decoupled from each other

joshua-kim · 2024-07-16T14:56:13Z

extensions/indexer/indexer.go

+type SuccessfulTxIndexer interface {
+	Accepted(ctx context.Context, tx *chain.Transaction, result *chain.Result) error
+}


I think if we go with the route of removing the metrics code out of indexer implementations we should be able to remove this interface as well

I'll move the actions to their own accepted subscriber as suggested here #1143 (comment)

Still leaving this extra interface in, so that each controller can pass in its own successful tx handler. The alternative would be for both of them to just implement the full interface. Either is fine imo, lmk if you'd prefer the latter.

extensions/indexer/indexer.go

extensions/indexer/storage.go

examples/tokenvm/tests/integration/integration_test.go

extensions/indexer/indexer.go

joshua-kim · 2024-07-16T15:18:55Z

extensions/indexer/indexer.go

+	Accepted(ctx context.Context, tx *chain.Transaction, result *chain.Result) error
+}
+
+type TxIndexer struct {


If we're going to make this an extension that other vms will pull in it probably makes sense to add unit tests for this

Wasn't sure if that should be part of this PR, but can add it

I think we should make adding a unit test a separate PR because you currently need a builder in order to construct a stateless block...

We need to clean a lot of this code up to make writing unit tests more reasonable, so I'd prefer to defer this to a separate PR and keep the scope of this one a little bit smaller.

aaronbuchwald · 2024-07-16T20:07:29Z

707dad3 fixes https://github.com/ava-labs/hypersdk/actions/runs/9963000960/job/27528179448?pr=1143#step:5:433

where I did not not invoke the combined subscriber correctly (still called the tx indexer instead)

aaronbuchwald · 2024-07-16T22:05:55Z

707dad3 fixes https://github.com/ava-labs/hypersdk/actions/runs/9963000960/job/27528179448?pr=1143#step:5:433
where I did not not invoke the combined subscriber correctly (still called the tx indexer instead)

Digging into this a bit more, this happens for the TokenVM tests when the actionIndexer is a subscriber registered AFTER the txIndexer. If I reverse the order, the tests pass reliably locally.

I can also fix this test by inserting a sleep statement between marking the block as accepted and invoking the tcli. It seems that this code is racey because controller.Accepted is actually called from a goroutine processes accepted blocks asynchronously:

hypersdk/vm/resolutions.go

Line 213 in a061629

for b := range vm.acceptedQueue {

.

This race condition is not introduced by this change, but it does appear to happen reliably with this change iff the actionIndexer comes second.

I think it's reasonable to swap the order, so that this passes CI and add a TODO calling out the race condition in the integration test.

…ition

joshua-kim · 2024-07-18T15:21:58Z

examples/morpheusvm/controller/action_indexer.go

+
+var _ indexer.SuccessfulTxSubscriber = (*actionHandler)(nil)
+
+type actionHandler struct {


Should we call this something else...? Something related metrics or something?

acceptedActionMetricsHandler? This is a bit too long imo and I slightly prefer actionHandler in this case unless you have an alternative suggestion.

joshua-kim · 2024-07-18T15:23:53Z

extension/indexer/tx_indexer.go

+
+type noopTxIndexer struct{}
+
+func NewNoopTxIndexer() *noopTxIndexer {


nit: NoOp

joshua-kim · 2024-07-18T15:24:10Z

extension/indexer/tx_indexer.go

+	return true, t, success, d, fee, nil
+}
+
+type noopTxIndexer struct{}


nit: export type so caller can use this type if it wants to. Applicable to rest of concrete-types in this package

I disagree with this. This is intentionally hiding that detail assuming callers choose to create this type and don't need to know the details or change anything about it after the fact.

Open to changing my mind on this though

If you don't want to let the caller know about the type for some reason, then you can just return the interface. Returning an unexported struct makes the type awkward to use. You can't embed, wrap, return a non-exported type from a calling package.

At the same time what's the disadvantage of exporting the type? Returning a concrete type gives more flexibility to the caller to use it either as the interface or as the concrete type. I think we should let consumers of the package decide how they want to use our api (this way they can use our indexer interface, wrap it with their own, or use the type).

joshua-kim · 2024-07-18T15:26:33Z

extension/indexer/accepted_subscriber.go

+	subscribers []AcceptedSubscriber
+}
+
+func NewCombinedAcceptedSubscriber(subscribers ...AcceptedSubscriber) *CombinedAcceptedSubscriber {


Can we call this function + this type func NewAcceptedSubscribers(...) *AcceptedSubscribers?

I feel like combined is more explicit than adding -s, which seems easier to miss.

joshua-kim · 2024-07-18T15:28:14Z

extension/indexer/tx_indexer.go

+	_ AcceptedSubscriber = (*noopTxIndexer)(nil)
+)
+
+var (


nit: combine the two var blocks

I generally prefer a separate var block for interface assertions, but don't see that used elsewhere in this repo and do see a combined var block with another variable used in one place.

Will update.

joshua-kim · 2024-07-18T15:29:53Z

examples/tokenvm/controller/action_indexer.go

+
+var _ indexer.SuccessfulTxSubscriber = (*actionHandler)(nil)
+
+type actionHandler struct {


Similar comment as the corresponding comment in morpheusvm

joshua-kim · 2024-07-18T15:31:46Z

examples/tokenvm/controller/action_indexer.go

+			a.c.orderBook.Add(chain.CreateActionID(tx.ID(), uint8(i)), tx.Auth.Actor(), action)
+		case *actions.FillOrder:
+			a.c.metrics.fillOrder.Inc()
+			outputs := result.Outputs[i]


Wondering if the order-book updates should happen in their own subscriber since they are logically decoupled from the metrics

I think it's fine to do more than one thing here.

Doing this async (as it's handled prior to this PR as well) does introduce a race condition because the in-memory order book is not updated during block accept. We may want to support a SyncAcceptedSubscriber and AsyncAcceptedSubscriber to fix that.

Yeah I think long term we're gonna try to get rid of the metrics in here and push it up into hypersdk anyways so... not too concerned

joshua-kim · 2024-07-18T15:35:16Z

extension/indexer/tx_indexer.go

+	GetTransaction(txID ids.ID) (bool, int64, bool, fees.Dimensions, uint64, error)
+}
+
+type txIndexer struct {


Could probably just call this DiskIndexer. Feels weird to have the concrete-type just be the name of the interface.

Renamed to txDBIndexer and renaming the constructor as well

joshua-kim · 2024-07-18T15:37:10Z

extension/indexer/tx_indexer.go

+)
+
+type TxIndexer interface {
+	Accepted(ctx context.Context, blk *chain.StatelessBlock) error


Probably should just replace this line w/ an embed for the AcceptedSubscriber interface it since that's the interface we're using to get access to accepted blocks

joshua-kim · 2024-07-18T15:39:09Z

extension/indexer/tx_indexer.go

+
+type TxIndexer interface {
+	Accepted(ctx context.Context, blk *chain.StatelessBlock) error
+	GetTransaction(txID ids.ID) (bool, int64, bool, fees.Dimensions, uint64, error)


Should we return a struct here instead? We can define an indexer.Tx struct type or something that the indexer defines for indexed tx data.

This is an interface, so we can swap a noop version in if tx indexing is disabled as per #1143 (comment)

joshua-kim · 2024-07-18T17:21:59Z

extension/indexer/accepted_subscriber.go

+	return &AcceptedSubscribers{subscribers: subscribers}
+}
+
+func (c *AcceptedSubscribers) Accepted(ctx context.Context, blk *chain.StatelessBlock) error {


nit: update to a since we updated this struct name

extension/indexer/successful_tx_subscriber.go

joshua-kim · 2024-07-18T17:46:16Z

extension/indexer/tx_indexer.go

+	return &txDBIndexer{db: db}
+}
+
+func (i *txDBIndexer) Accepted(_ context.Context, blk *chain.StatelessBlock) error {


nit: export the struct + rename the receiver

joshua-kim · 2024-07-18T17:47:35Z

extension/indexer/accepted_subscriber.go

+)
+
+type AcceptedSubscriber interface {
+	Accepted(ctx context.Context, blk *chain.StatelessBlock) error


I don't think we need this context + in the other tx subscriber interface currently... we could leave it in since I think it's reasonable that you would want a context though (say if you were archiving into s3 or something).

Ya I think this is reasonable to leave in as part of the interface

* Remove rejected from controller interface * Update mocks and remove unused mock_auth from mocks.mockgen.txt * Refactor tx indexing into indexer extension * Close txDB in morpheusvm on shutdown * Update name of unused result param to _ * fix lint * Update license year * Address nits * move action handler to its own accepted subscriber * Add interface check for noopTxIndexer * Fix typo * fix unused receivers * fix linting * invoke combined subscriber to fix integration test * move action handler prior to tx indexer to fix pre-existing race condition * Add TODO to integration test flagging flaky test * Noop -> NoOp * Address comments * Export NoOpTxIndexer * rename combinedAcceptedSubscriber AcceptedSubscribers

…eful shutdown * keep track of the last accepted block at the end of the `processAcceptedBlocks` func * handle error and panic * Create a new processed key and retrieve it to send to websocket if present on boot * use `lastProcessed` key * update metrics on block retrieval * set processed height at the end of processing and process block on reboot for each unprocessed block * remove processed block function is useless * break the block into a function and add blocks to the chan * add logs, return fatal error, fix cache miss * set bounds on `AcceptorSize` and `AcceptedBlockWindow` * [cli/spam] Incorporate `vryx-poc` Improvements (#1091) * fix withholding spelling * define prompt float * remove useless const * more cleanup * making progress * working on issue loop * turn this into an interface rather than providing this functions * making progress on core loop * code is compiling * bound inflight returns with channel * cleanup struct * remove resolved todo * update transfer to include memo * cleanup morephus cli definition * remove unused variables * cleanup tokenvm client * add issuance limiter * fix errors * don't add tx to block if too big * put more txs in flight * cleanup logging * add fix for fees * track outstanding txs * fix var order * fix returned balance * test fix * linter * add comments * resolve linter for morpheusvm * pass tokenvm linter * add memo to tx size * gci imports * trying again * integration tests now passing * Add TODO to fix race conditions in spam script --------- Co-authored-by: Aaron Buchwald <aaron.buchwald56@gmail.com> * Extract shared scripts (#1161) * chore: rebase on top of master * refactor: shellcheck * fix: update mockgen version to pass ci checks * fix: build script merge * fix: resolve issues in tests.lint.sh * fix: resolve imports and shellcheck issues * fix: shellcheck issues * fix: downgrade golangci-lint to pass ci * fix: separate unit testing scripts * fix: ci issues * fix: ci issues * fix: tokenvm static analysis * fixup: Remove merge conflicts --------- Co-authored-by: vikinatora <vikinatora@gmail.com> * fix comment (#1163) * Add commit signing as instruction in contributing.md (#1164) * Add Tx Indexer Extension (#1143) * Remove rejected from controller interface * Update mocks and remove unused mock_auth from mocks.mockgen.txt * Refactor tx indexing into indexer extension * Close txDB in morpheusvm on shutdown * Update name of unused result param to _ * fix lint * Update license year * Address nits * move action handler to its own accepted subscriber * Add interface check for noopTxIndexer * Fix typo * fix unused receivers * fix linting * invoke combined subscriber to fix integration test * move action handler prior to tx indexer to fix pre-existing race condition * Add TODO to integration test flagging flaky test * Noop -> NoOp * Address comments * Export NoOpTxIndexer * rename combinedAcceptedSubscriber AcceptedSubscribers * [cli/spam] Incorporate `vryx-poc` Improvements (#1091) * fix withholding spelling * define prompt float * remove useless const * more cleanup * making progress * working on issue loop * turn this into an interface rather than providing this functions * making progress on core loop * code is compiling * bound inflight returns with channel * cleanup struct * remove resolved todo * update transfer to include memo * cleanup morephus cli definition * remove unused variables * cleanup tokenvm client * add issuance limiter * fix errors * don't add tx to block if too big * put more txs in flight * cleanup logging * add fix for fees * track outstanding txs * fix var order * fix returned balance * test fix * linter * add comments * resolve linter for morpheusvm * pass tokenvm linter * add memo to tx size * gci imports * trying again * integration tests now passing * Add TODO to fix race conditions in spam script --------- Co-authored-by: Aaron Buchwald <aaron.buchwald56@gmail.com> * Extract shared scripts (#1161) * chore: rebase on top of master * refactor: shellcheck * fix: update mockgen version to pass ci checks * fix: build script merge * fix: resolve issues in tests.lint.sh * fix: resolve imports and shellcheck issues * fix: shellcheck issues * fix: downgrade golangci-lint to pass ci * fix: separate unit testing scripts * fix: ci issues * fix: ci issues * fix: tokenvm static analysis * fixup: Remove merge conflicts --------- Co-authored-by: vikinatora <vikinatora@gmail.com> * fix comment (#1163) * Add commit signing as instruction in contributing.md (#1164) * Add Tx Indexer Extension (#1143) * Remove rejected from controller interface * Update mocks and remove unused mock_auth from mocks.mockgen.txt * Refactor tx indexing into indexer extension * Close txDB in morpheusvm on shutdown * Update name of unused result param to _ * fix lint * Update license year * Address nits * move action handler to its own accepted subscriber * Add interface check for noopTxIndexer * Fix typo * fix unused receivers * fix linting * invoke combined subscriber to fix integration test * move action handler prior to tx indexer to fix pre-existing race condition * Add TODO to integration test flagging flaky test * Noop -> NoOp * Address comments * Export NoOpTxIndexer * rename combinedAcceptedSubscriber AcceptedSubscribers * Update vm/vm.go Co-authored-by: aaronbuchwald <aaron.buchwald56@gmail.com> Signed-off-by: Franfran <51274081+iFrostizz@users.noreply.github.com> * wrap err and fix restoring off-by-one * fix operator inverted * off-by-one on amount of accepted to restore * typo * Update vm/vm.go Co-authored-by: aaronbuchwald <aaron.buchwald56@gmail.com> Signed-off-by: Franfran <51274081+iFrostizz@users.noreply.github.com> * Update vm/vm.go Co-authored-by: aaronbuchwald <aaron.buchwald56@gmail.com> Signed-off-by: Franfran <51274081+iFrostizz@users.noreply.github.com> * use constants for MaxAcceptorSize and MinAcceptedBlockWindow and update error messages * add provided value in error message --------- Signed-off-by: Franfran <51274081+iFrostizz@users.noreply.github.com> Co-authored-by: Patrick O'Grady <prohb125@gmail.com> Co-authored-by: Aaron Buchwald <aaron.buchwald56@gmail.com> Co-authored-by: marun <maru.newby@avalabs.org> Co-authored-by: vikinatora <vikinatora@gmail.com> Co-authored-by: PolyMa <151764357+polymaer@users.noreply.github.com>

aaronbuchwald added 4 commits July 15, 2024 09:21

Remove rejected from controller interface

d613350

Update mocks and remove unused mock_auth from mocks.mockgen.txt

aeb04bd

Refactor tx indexing into indexer extension

0b4167c

Close txDB in morpheusvm on shutdown

a776c7d

aaronbuchwald marked this pull request as draft July 15, 2024 15:53

Update name of unused result param to _

5806dc1

aaronbuchwald changed the base branch from remove-rejected-controller to main July 15, 2024 21:35

aaronbuchwald added 3 commits July 15, 2024 17:36

Merge branch 'main' into tx-indexer

496f4d4

fix lint

5fe1ac0

Update license year

4df39fa

aaronbuchwald marked this pull request as ready for review July 15, 2024 22:08

aaronbuchwald requested a review from joshua-kim July 15, 2024 22:08

aaronbuchwald self-assigned this Jul 15, 2024

aaronbuchwald mentioned this pull request Jul 15, 2024

Fix Accepted/Rejected reliability #1145

Closed

joshua-kim reviewed Jul 16, 2024

View reviewed changes

aaronbuchwald added 8 commits July 16, 2024 14:26

Address nits

0303667

move action handler to its own accepted subscriber

7236bd2

Add interface check for noopTxIndexer

4c365aa

Fix typo

9dd485b

fix unused receivers

cff7da7

fix linting

6c925f7

Merge branch 'main' into tx-indexer

6b516e2

invoke combined subscriber to fix integration test

707dad3

aaronbuchwald added 2 commits July 16, 2024 18:10

move action handler prior to tx indexer to fix pre-existing race cond…

2e6067f

…ition

Add TODO to integration test flagging flaky test

0770055

aaronbuchwald mentioned this pull request Jul 17, 2024

Decompose VM Controller #1159

Closed

Merge branch 'main' into tx-indexer

e0c3c68

aaronbuchwald requested a review from joshua-kim July 17, 2024 18:52

aaronbuchwald mentioned this pull request Jul 18, 2024

S3 Archiver in TokenVM + RPC #697

Closed

joshua-kim reviewed Jul 18, 2024

View reviewed changes

aaronbuchwald added 5 commits July 18, 2024 11:41

Noop -> NoOp

9850970

Address comments

650572e

Merge branch 'main' into tx-indexer

1ccb8a2

Export NoOpTxIndexer

f9d658f

rename combinedAcceptedSubscriber AcceptedSubscribers

6296694

aaronbuchwald enabled auto-merge (squash) July 18, 2024 17:48

aaronbuchwald mentioned this pull request Jul 18, 2024

[rpc] optionally storing block.Results on disk, start stream from a specified block #692

Closed

7 tasks

joshua-kim reviewed Jul 18, 2024

View reviewed changes

address comments

8a06837

joshua-kim approved these changes Jul 18, 2024

View reviewed changes

aaronbuchwald merged commit c348296 into main Jul 18, 2024
20 checks passed

joshua-kim deleted the tx-indexer branch July 18, 2024 20:47

This was referenced Jul 30, 2024

Combine APIs + Controller #1224

Closed

Support External Indexer via gRPC AcceptedSubscriber + Optional Internal Indexing #1225

Open


		var _ indexer.SuccessfulTxIndexer = &successfulTxIndexer{}

		type successfulTxIndexer struct {


		var _ indexer.SuccessfulTxSubscriber = (*actionHandler)(nil)

		type actionHandler struct {


		type noopTxIndexer struct{}

		func NewNoopTxIndexer() *noopTxIndexer {

Add Tx Indexer Extension #1143

Add Tx Indexer Extension #1143

Conversation

aaronbuchwald commented Jul 15, 2024

joshua-kim Jul 16, 2024 • edited Loading

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

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

aaronbuchwald commented Jul 16, 2024

aaronbuchwald commented Jul 16, 2024

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshua-kim Jul 16, 2024 •

edited

Loading