-
Notifications
You must be signed in to change notification settings - Fork 120
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
tapfreighter: don't send proofs for tombstones, remove tombstones and burns from commitments #556
Conversation
b1a7c70
to
d4a2b8f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to see the variable renames 👍
@@ -452,18 +452,14 @@ func (c *TapCommitment) Merge(other *TapCommitment) error { | |||
|
|||
// Otherwise, we'll need to merge the other asset commitments into | |||
// this commitment. | |||
for key, otherCommitment := range other.assetCommitments { | |||
existingCommitment, ok := c.assetCommitments[key] | |||
for key := range other.assetCommitments { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key
might not be the correct variable name after this change. I think it's an index now. Should we just use i
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, key
is still correct IMO, since assetCommitments
is a map, not a slice.
Because grouped assets will have the same Tap level key, the NewTapCommitments function will overwrite all grouped assets one after each other instead of merging them. So we use the more easy to use FromAssets function instead. We'll fix the behavior of NewTapCommitments in the next commit.
This commit fixes an issue with the NewTapCommitment function which used to overwrite asset commitments if they had the same Taproot Asset commitment key (which is true for grouped assets for example).
The AssetCommitment had a field called AssetID that was quite confusing and misleading because for grouped assets this would be the hashed group key instead of the asset ID. To make the use of the field more clear and to avoid bugs (and actually fix a bug in a test), we rename the field to TapKey.
Don't attempt to send proofs for tombstone outputs.
We called ParseAndSetDebugLevels too early, so the log level config/CLI flag didn't have any effect. Also, we're going to add some trace statements to the tapscript package in the next commit, so we prepare by adding a package logger.
This is an area that is frequently visited when debugging commitment based problems. To improve clarity on some of the variable names, we perform a series of renames. We also add in some trace level log statments. This commit does NOT change any behavior in the code (other than logging).
This fixes a bug that we only discovered after sending assets many times back and forth. We didn't properly remove tombstones and burns from the commitments when creating new outputs. We did have tests that sent passive assets out from a commitment that had a tombstone. But we never spent that second output, which would have discovered the bug earlier. We'll fix this with a new integration test case in the next commit.
d4a2b8f
to
99fc3e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great set of fixes and readability improvements from older sections 🚀
This test reproduced the bug fixed in the previous commit and now confirms the fix is working. This test also asserts the fix for the problem with attempting to deliver a proof for a tombstone that was done in an earlier commit.
99fc3e4
to
b1084e8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🧨
for _, asset := range assets { | ||
asset := asset | ||
assetCommitments := make(AssetCommitments, len(newCommitments)) | ||
for idx := range newCommitments { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need to do this any longer, but does't hurt.
@@ -474,12 +470,7 @@ func (c *TapCommitment) Merge(other *TapCommitment) error { | |||
|
|||
// With either the new or merged asset commitment obtained, we | |||
// can now (re-)insert it into the Taproot Asset commitment. | |||
existingCommitmentCopy, err := existingCommitment.Copy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC we started to do this mainly as a precautionary mechanism.
Don't attempt to send proofs for tombstone outputs.
This wasn't discovered before because we don't have the proof courier enabled by default in all our integration tests.
EDIT: I pulled in the bug fix commits from #553 since it makes more sense to have them all bundled in the same PR.
So this PR now contains all fixes for bugs recently discovered and the unit or integration test to cover them.