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.

Sign up for GitHub

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

Jump to bottom

caretaker: make batch state atomic #323

Merged
merged 2 commits into from
May 30, 2023
Merged

caretaker: make batch state atomic #323

merged 2 commits into from
May 30, 2023

Conversation

jharveyb
Copy link
Contributor

Fixes #296.

This was the cleanest way I saw to make these changes; we can't have BatchState as a type alias for atomic.Uint32, since then we can't define batch states with an enum.

Confirmed that this fixes make unit-race failing on my machine.

@jharveyb jharveyb requested review from guggero and ffranr May 25, 2023 14:30
guggero
guggero reviewed May 25, 2023
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Nice change!

tapdb/asset_minting.go Outdated Show resolved Hide resolved
tapdb/asset_minting.go Outdated Show resolved Hide resolved
tapgarden/batch.go Show resolved Hide resolved
tapgarden/interface.go Outdated Show resolved Hide resolved
tapgarden/interface.go Outdated Show resolved Hide resolved
tapgarden/interface.go Outdated Show resolved Hide resolved
tapgarden/interface.go Outdated Show resolved Hide resolved
tapgarden/interface.go Outdated Show resolved Hide resolved
@jharveyb
Copy link
Contributor Author

New flake (?), seems important:

2023-05-25 14:33:39.732 [INF] UNIV: UniverseRoot(2fb56e101d8ea37759c5d5454153dd866ed43e9c268af9006aab2e29c9c6cccd): inserting new leaf
2023-05-25 14:33:39.733 [WRN] UNIV: unable to log sync event: unknown postgres error: ERROR: null value in column "universe_root_id" violates not-null constraint (SQLSTATE 23502)
2023-05-25 14:33:39.745 [WRN] UNIV: unable to log new proof event: unknown postgres error: ERROR: null value in column "universe_root_id" violates not-null constraint (SQLSTATE 23502)
2023-05-25 14:33:39.745 [ERR] RPCS: [/universerpc.Universe/SyncUniverse]: unable to sync universe: unable to register issuance proof: unable to register new issuance: sql unique constraint violation: ERROR: duplicate key value violates unique constraint "mssmt_nodes_pkey" (SQLSTATE 23505)
    test_harness.go:231: 
        	Error Trace:	/home/runner/work/taproot-assets/taproot-assets/itest/test_harness.go:231
        	            				/home/runner/work/taproot-assets/taproot-assets/itest/test_harness.go:377
        	            				/home/runner/work/taproot-assets/taproot-assets/itest/multi_asset_group_test.go:122
        	            				/home/runner/work/taproot-assets/taproot-assets/itest/test_harness.go:162
        	            				/home/runner/work/taproot-assets/taproot-assets/itest/integration_test.go:71
        	Error:      	Received unexpected error:
        	            	rpc error: code = Unknown desc = unable to sync universe: unable to register issuance proof: unable to register new issuance: sql unique constraint violation: ERROR: duplicate key value violates unique constraint "mssmt_nodes_pkey" (SQLSTATE 23505)
        	Test:       	TestTaprootAssetsDaemon/minting_multi_asset_groups
2023-05-25 14:33:40.565 [ERR] GRDN: Aborting main custodian event loop: receive failed: EOF

@jharveyb jharveyb force-pushed the caretaker_atomic_batch_state branch from 2693f4a to 665c000 Compare May 25, 2023 15:34
@jharveyb jharveyb mentioned this pull request May 25, 2023
Closed
jharveyb added 2 commits May 25, 2023 11:45
@jharveyb
tapgarden: use private atomic uint for batch state
ac1b661 
In this commit, we change the MintingBatch.BatchState variable from a
public uint32 to a private atomic.uint32, to prevent race conditions
when cancelling a batch. We also add methods to read and write this
private batch state, and new logic to check that plain uint32s are valid
BatchStates.
@jharveyb
rpcserver+tapdb: use new batch state R/W methods
837b930
@jharveyb jharveyb force-pushed the caretaker_atomic_batch_state branch from 665c000 to 837b930 Compare May 25, 2023 15:57
ffranr
ffranr approved these changes May 30, 2023
View reviewed changes
Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

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

Good idea! 👍

I think a single commit would do, but it doesn't really matter.

@guggero guggero added this pull request to the merge queue May 30, 2023
Merged via the queue into main with commit 97c045e May 30, 2023
@jharveyb jharveyb deleted the caretaker_atomic_batch_state branch July 5, 2023 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ffranr ffranr ffranr approved these changes

@guggero guggero guggero approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tarogarden: caretaker data race
3 participants
@jharveyb @guggero @ffranr