Spend transaction change outputs even if undelivered proof(s) #1074

ffranr · 2024-08-09T14:37:23Z

Refactor ChainPorter state machine such that change outputs can be spent even if proof delivery is incomplete.

Changes

Integration Test: Spend Change on Failed Proof Delivery
- A new integration test is added to ensure that a tapd node can spend a change output even if the proof transfer for the previous transaction fails.
Renaming Methods and States for Clarity
- The ConfirmParcelDelivery method is renamed to LogAnchorTxConfirm to more accurately reflect its functionality. This method now focuses on updating the send package on disk to indicate the anchoring transaction's on-chain confirmation, without "confirming parcel delivery" to disk (it never did that anyway).
- The ChainPorter state is renamed from SendStateStoreProofs to SendStateStorePostAnchorTxConf. This new name better represents the broader storage updates performed after the anchor transaction confirmation, beyond just proof storage.
State Management Enhancements
- The ChainPorter state machine is updated to store the package state after the anchor transaction confirmation, but before initiating the proof transfer. This change ensures that the transfer change output coins remain spendable even if the proof delivery fails.
- The anchor transaction block hash is now stored in the outbound package, providing additional context for the state of pending transfers.
Database Query Generalization
- The QueryAssetTransfers SQL query argument unconf_only is generalized and renamed to pending_transfers_only. This updated flag now includes pending transfers with unconfirmed anchoring transactions or undelivered transfer output proofs. The query now also returns the anchor transaction block hash, allowing users to determine if the transfer's anchoring transaction has been confirmed on-chain.

ffranr · 2024-08-09T14:54:08Z

FAIL: TestTaprootAssetsDaemon/fee_estimation just failed. That test might be flaky. I don't think it's related to the changes in this PR.

guggero

Very nice!

tapdb/sqlc/queries/transfers.sql

guggero · 2024-08-12T10:44:13Z

tapdb/sqlc/queries/transfers.sql

@@ -42,20 +42,25 @@ SET proof_delivery_complete = @delivery_complete
 WHERE output_id = (SELECT output_id FROM target);

 -- name: QueryAssetTransfers :many
-SELECT
-    id, height_hint, txns.txid, transfer_time_unix
+SELECT DISTINCT


Why is the distinct needed here? That's usually an indication that a JOIN isn't correct. Perhaps the asset_transfer_outputs should not be a JOIN but rather be a sub query with a AND EXISTS(select 1 from asset_transfer_outputs ...)?

DISTINCT is used because of the join on asset_transfer_outputs. There are many other ways of accomplishing the same thing. Why should we disfavour DISTINCT?

I'm just saying that whenever I see a DISTINCT in a query, from experience, it's a sign that most likely something is fishy with the query. We're joining over an additional table, even though we don't need values from that table, so it should probably only be used in a sub-query (or maybe a CTE would fit here as well).
Also, I could imagine DISTINCT not being super fast, but that's more of a gut feeling.

Also, I think we should have a basic unit tests covering the new query. I myself made multiple mistakes in queries just because I didn't add proper unit tests.

Thinking about this a bit more, perhaps in general we want both DISTINCT and a better join? Here's my reasoning:

Without DISTINCT we face two errors: duplicates and missing rows. But with DISTINCT we only have to worry about missing rows. In other words, we do actually want the final rows to be DISTINCT. And it might be good for robustness.

As for performance, I think we need to optimise against an actual problem, and I don't think we've seen a performance problem just yet. And it's not clear to me that performance is actually significantly affected.

But I also see your point that the rest of the query has to remove duplicates also. Otherwise we might lose precision in what we want the statement to achieve.

Do you think using DISTINCT will hide problems though?

I also don't understand what the DISTINCT is doing here - how would we end up with duplicate or missing rows for this query?

The OR clause inside the AND also feels weird - I thought we have some other pattern to handle an optional filter like this that was more readable (but I couldn't find it with a cursory search).

Yeah, I'm pretty sure we don't need the DISTINCT anymore with the sub query. Before it was needed to de-duplicate because we selected/joined transfers and outputs but were only interested in fields of the transfers, so duplicates were possible.
But now we only select over transfers (and chain transactions, which should be a 1:1 relationship). So there shouldn't be any duplicates.
And I'm not sure what you mean by "missing rows"? Missing from what table?

And I'm not sure what you mean by "missing rows"? Missing from what table?

I mean that one error type is that the query doesn't return some rows that should be there. And that DISTINCT guarantees the absence of another error type: duplicate rows. It's nice to have that guarantee.

I agree that we don't need DISTINCT but we might as well have it because it is what we want. We want the returned rows from the query to be distinct.

Anyway, I'm going to remove DISTINCT for now because I don't think it matters very much. The JOIN has been modified as you suggested.

itest/send_test.go

coveralls · 2024-08-12T13:48:14Z

Pull Request Test Coverage Report for Build 10373657954

Details

8 of 101 (7.92%) changed or added relevant lines in 5 files are covered.
39 unchanged lines in 8 files lost coverage.
Overall coverage decreased (-0.06%) to 40.16%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tapfreighter/parcel.go	0	2	0.0%
tapdb/assets_store.go	6	13	46.15%
rpcserver.go	0	19	0.0%
tapfreighter/chain_porter.go	0	65	0.0%

Files with Coverage Reduction	New Missed Lines	%
tappsbt/create.go	2	53.85%
asset/asset.go	2	81.73%
tapdb/universe.go	4	80.91%
tapgarden/caretaker.go	4	68.5%
commitment/tap.go	4	84.17%
rpcserver.go	4	0.0%
tapdb/multiverse.go	7	60.32%
universe/interface.go	12	50.22%

Totals
Change from base Build 10373449798:	-0.06%
Covered Lines:	23678
Relevant Lines:	58959

💛 - Coveralls

ffranr · 2024-08-12T15:50:09Z

This PR also adds extra useful fields to the ListTransfers RPC endpoint response.

jharveyb

Looks generally good! Seems like the follow-up PR to assert that restart works should be pretty small.

Only outstanding Q is on the DB query changes.

tapdb/assets_store_test.go

tapfreighter/chain_porter.go

itest/send_test.go

jharveyb

Updates look good! Just blocking on that one unit test update.

tapdb/assets_store_test.go

This commit renames and generalizes the `unconf_only` argument in the `QueryAssetTransfers` SQL query to `pending_transfers_only`. The flag now returns pending transfers with unconfirmed anchoring transactions or undelivered transfer output proofs. Additionally, this update includes the anchor transaction block hash in the query's returned values. This allows users to determine if a transfer's anchoring transaction has been confirmed on-chain, providing context for why the transfer is pending. As a result of this change, ExportLog.PendingParcels will now also include transfers that are pending due to undelivered proofs, even if their associated anchoring transactions have already been confirmed on-chain.

This commit modifies the `ChainPorter` state machine to store the package state after the anchor transaction is confirmed, but before proof transfer is initiated. By ensuring the state is saved at this point, transfer change output coins remain spendable even if proof delivery fails.

This commit renames the ChainPorter state from SendStateStoreProofs to SendStateStorePostAnchorTxConf. The new name more accurately represents its function, reflecting that this state now performs broader storage updates beyond just storing proofs. Additionally, the name indicates that this state is triggered after the confirmation of the transfer anchoring transaction on-chain, without delving into the specific details of the operations performed within the state.

This commit renames the ExportLog method from ConfirmParcelDelivery to LogAnchorTxConfirm. The purpose of this change is to clarify that the method does not confirm parcel delivery to disk, as transfer proofs may still be pending. Instead, it updates the send package on disk to indicate that the transfer anchoring transaction has been confirmed on-chain.

This commit introduces the anchor transaction block hash as a field in the `AssetTransfer` RPC message. The `ListTransfers` RPC endpoint now includes this block hash for each asset transfer, provided the anchor transaction is confirmed. If the transaction is unconfirmed, this field will remain unset.

This commit enhances the ListTransfers RPC endpoint by adding the proof delivery status for each transfer output in its response.

This commit adds an itest which ensures that a tapd node is able to spend a change output even if the proof transfer for the previous transaction fails.

jharveyb

LGTM 👍🏽

guggero

Very nice, LGTM 🎉

One tiny request, but can also be deferred to follow-up PR, so non-blocking.

guggero · 2024-08-14T07:55:55Z

taprpc/taprootassets.proto

+
+    // The block hash the contains the anchor transaction above. If unset, the
+    // anchor transaction is unconfirmed.
+    bytes anchor_tx_block_hash = 7;


Same request as here: #1075 (comment)
Whenever we reference block or transaction hashes, I think we should also return the human-readable (byte-reversed) variant that can be copy/pasted easily from the command line. So just add an additional anchor_tx_block_hash_str that returns the .String() variant.

OK, I'll add this in a future PR (you mention in the review that you're ok with that). I think I've got a good pattern in mind.

ffranr requested review from guggero and jharveyb August 9, 2024 14:37

ffranr self-assigned this Aug 9, 2024

guggero reviewed Aug 12, 2024

View reviewed changes

ffranr force-pushed the spend-change-undelivered-proof branch from bc5cde5 to 5b211ff Compare August 12, 2024 13:38

ffranr force-pushed the spend-change-undelivered-proof branch from 5b211ff to 3dfecbe Compare August 12, 2024 15:34

dstadulis added this to the v0.4.2 milestone Aug 12, 2024

ffranr force-pushed the spend-change-undelivered-proof branch from 3dfecbe to 8338e95 Compare August 12, 2024 15:48

ffranr requested a review from guggero August 12, 2024 16:04

dstadulis added the proofs label Aug 12, 2024

jharveyb reviewed Aug 12, 2024

View reviewed changes

ffranr force-pushed the spend-change-undelivered-proof branch 3 times, most recently from 2a40b27 to fa4a66d Compare August 13, 2024 11:38

ffranr requested a review from jharveyb August 13, 2024 11:56

ffranr mentioned this pull request Aug 13, 2024

[feature]: Add integration test for concurrent transfers with delayed block mining #1081

Open

jharveyb reviewed Aug 13, 2024

View reviewed changes

tapdb/assets_store_test.go Show resolved Hide resolved

ffranr added 8 commits August 13, 2024 17:31

tapfreighter: store anchor tx block hash in outbound package

ef90f31

taprpc: add proof delivery status field to RPC TransferOutput message

99daf3d

This commit enhances the ListTransfers RPC endpoint by adding the proof delivery status for each transfer output in its response.

itest: spend change on failed proof delivery

6f0f23f

This commit adds an itest which ensures that a tapd node is able to spend a change output even if the proof transfer for the previous transaction fails.

ffranr force-pushed the spend-change-undelivered-proof branch from fa4a66d to 6f0f23f Compare August 13, 2024 16:34

ffranr requested a review from jharveyb August 13, 2024 16:35

jharveyb approved these changes Aug 13, 2024

View reviewed changes

guggero approved these changes Aug 14, 2024

View reviewed changes

ffranr added this pull request to the merge queue Aug 14, 2024

Merged via the queue into main with commit 8fcd27c Aug 14, 2024
17 checks passed

guggero deleted the spend-change-undelivered-proof branch August 14, 2024 10:22

jharveyb mentioned this pull request Aug 14, 2024

tapgarden/[bug]: batch not marked as confirmed before universe proof delivery #1086

Open

ffranr mentioned this pull request Aug 14, 2024

rpc: add ChainHash message with both bytes and string representations #1087

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spend transaction change outputs even if undelivered proof(s) #1074

Spend transaction change outputs even if undelivered proof(s) #1074

ffranr commented Aug 9, 2024

ffranr commented Aug 9, 2024

guggero left a comment

guggero Aug 12, 2024

ffranr Aug 12, 2024 •

edited

Loading

guggero Aug 12, 2024

guggero Aug 12, 2024

ffranr Aug 12, 2024 •

edited

Loading

jharveyb Aug 12, 2024 •

edited

Loading

guggero Aug 13, 2024

ffranr Aug 13, 2024

coveralls commented Aug 12, 2024 •

edited

Loading

ffranr commented Aug 12, 2024

jharveyb left a comment

jharveyb left a comment

jharveyb left a comment

guggero left a comment

guggero Aug 14, 2024

ffranr Aug 14, 2024

Spend transaction change outputs even if undelivered proof(s) #1074

Spend transaction change outputs even if undelivered proof(s) #1074

Conversation

ffranr commented Aug 9, 2024

Changes

ffranr commented Aug 9, 2024

guggero left a comment

Choose a reason for hiding this comment

guggero Aug 12, 2024

Choose a reason for hiding this comment

ffranr Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

guggero Aug 12, 2024

Choose a reason for hiding this comment

guggero Aug 12, 2024

Choose a reason for hiding this comment

ffranr Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

jharveyb Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

guggero Aug 13, 2024

Choose a reason for hiding this comment

ffranr Aug 13, 2024

Choose a reason for hiding this comment

coveralls commented Aug 12, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10373657954

Details

💛 - Coveralls

ffranr commented Aug 12, 2024

jharveyb left a comment

Choose a reason for hiding this comment

jharveyb left a comment

Choose a reason for hiding this comment

jharveyb left a comment

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

guggero Aug 14, 2024

Choose a reason for hiding this comment

ffranr Aug 14, 2024

Choose a reason for hiding this comment

ffranr Aug 12, 2024 •

edited

Loading

ffranr Aug 12, 2024 •

edited

Loading

jharveyb Aug 12, 2024 •

edited

Loading

coveralls commented Aug 12, 2024 •

edited

Loading