tapgarden: copy MintingBatches defensively

[jtobin]

The 'pendingBatches' here are references that are updated by the gardener goroutine. Previously the references themselves were handed to callers, meaning that callers could have inconsistent views of data if a reference was updated by the gardener. This commit merely copies each 'pendingBatch' before passing it on.

This may alleviate one or both of #1882 and #1885, as I expect this to improve the semantics of assertPendingBatchExists in planter_test.go in particular (and thus queueInitialBatch, testMintingCancelFinalize, testFundSealBeforeFinalize), but it seems an improvement regardless.

[gemini-code-assist]

Summary of Changes

Hello @jtobin, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the robustness of the tapgarden component by implementing defensive copying for MintingBatch objects. The core purpose is to eliminate potential concurrency issues where external callers might receive direct references to internal, mutable state, leading to inconsistent data. By ensuring that copies are provided instead of direct references, the system's data integrity and predictability are significantly improved, particularly in multi-threaded environments.

Highlights

• Defensive Copying of MintingBatches: The pull request introduces defensive copying of MintingBatch objects before they are passed to callers or resolved in requests. This prevents callers from holding direct references to mutable internal state, thereby avoiding potential concurrent read/write issues and ensuring data consistency.
• Addressing Concurrency Issues: By copying pendingBatch instances, the changes aim to mitigate race conditions and inconsistent data views that could arise when the gardener goroutine updates these batches while other parts of the system are accessing them. This is expected to improve the reliability of related test cases and overall system stability.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces defensive copying of MintingBatch instances before they are passed to other parts of the system, which is a good practice to prevent race conditions and unexpected mutations. The changes in gardener() correctly apply this pattern when handling seedlingReqs and stateReqs.

However, I've identified a potential issue with the MintingBatch.Copy() method itself. It performs a shallow copy of the Seedlings map, which means that while the map itself is new, it contains pointers to the same Seedling objects as the original. Any modification to these Seedling objects will be reflected in both the original and the copied batch. I've left a specific comment on this. While fixing the Copy method is likely outside the scope of this PR, it's an important consideration for making these copies truly defensive.

[coveralls]

Pull Request Test Coverage Report for Build 19963225879

Details

• 21 of 22 (95.45%) changed or added relevant lines in 2 files are covered.
• 57 unchanged lines in 14 files lost coverage.
• Overall coverage increased (+0.02%) to 56.616%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tapgarden/planter.go	19	20	95.0%

Files with Coverage Reduction	New Missed Lines	%
asset/asset.go	2	80.78%
asset/group_key.go	2	72.15%
lndservices/block_header_cache.go	2	97.52%
mssmt/compacted_tree.go	2	79.49%
tapdb/multiverse.go	2	80.69%
tapdb/universe.go	2	81.27%
tapgarden/custodian.go	2	76.83%
asset/mock.go	3	72.77%
itest/multisig.go	3	97.94%
universe/archive.go	3	78.54%

Totals
Change from base Build 19960793390:	0.02%
Covered Lines:	64854
Relevant Lines:	114550

---

💛 - Coveralls

[jtobin]

The CI jobs for unit-race and unit-cover, which frequently trigger tapgarden flakes, at least worked the first time here, which is encouraging. (EDIT: and again, on force push!)

[ffranr]

Maybe we need additional unit test coverage for Copy() somewhere?

[ffranr]

We can look into this further in a future PR.