-
Notifications
You must be signed in to change notification settings - Fork 133
supplyverifier: store and verify non-issuer supply pre-commitments #1784
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
supplyverifier: store and verify non-issuer supply pre-commitments #1784
Conversation
Rename SQL query to better align with "supply" terminology.
Rename SQL query UpsertMintAnchorUniCommitment to UpsertMintSupplyPreCommit. This gives room to add UpsertSupplyPreCommit in a later commit. Also, better align with "supply" terminology.
Rename SQL query FetchUnspentPrecommits to FetchUnspentMintSupplyPreCommits. This adds the term "Mint" to indicate that these supply pre-commitment entries relate to the local node's minting process. The rename also allows room to add FetchUnspentSupplyPreCommits in a subsequent commit.
Rename SQL query MarkPreCommitmentSpentByOutpoint to MarkMintPreCommitSpentByOutpoint. This adds the term "Mint" to indicate that these supply pre-commitment entries relate to the local node's minting process. The rename also allows room to add MarkPreCommitSpentByOutpoint in a subsequent commit.
Rename SQL table mint_anchor_uni_commitments to mint_supply_pre_commits. This aligns with the "supply" terminology and provides room to add a new supply pre-commits table for assets issued by a remote peer in a subsequent commit.
Extend FetchMintSupplyPreCommits to also return the supply pre-commit outpoint.
Pull Request Test Coverage Report for Build 17626735389Details
💛 - Coveralls |
func (s SupplySubTree) ToUniverseProofType() (universe.ProofType, error) { | ||
switch s { | ||
case MintTreeType: | ||
return universe.ProofTypeMintSupply, nil |
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.
In order to unify things further, perhaps we update the String
method of SupplySubTree
to just return a call to the String()
method of the corresponding ProofType
.
This way these strings aren't defined in two palces as they are now.
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.
Ah nice, good idea.
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.
hmm ToUniverseProofType()
returns an error where as SupplySubTree.String()
does not. That's causing some problems. Would you mind if I put up a separate PR to refactor SupplySubTree
perhaps?
@@ -326,6 +342,76 @@ func (s *SupplyCommitMachine) UnspentPrecommits(ctx context.Context, | |||
preCommits = append(preCommits, preCommit) | |||
} | |||
|
|||
// If any pre-commits were found where we acted as the issuer, |
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.
Perhaps this should just be another method? So one used for remote issuers, and one used for local issuers.
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.
I’ve been considering that option. My reasoning for keeping a single UnspentPrecommits
is twofold:
- There shouldn’t be any overlap between remote- and locally-issued assets, so
UnspentPrecommits
can be called safely in either context. - Splitting into local/remote variants adds naming complexity. Terms like “remote pre-commit” aren’t self-explanatory, and including “issued” in the names makes them cumbersome.
What do you rekon i should do here?
8ffa1eb
to
306bf35
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.
This all looks very solid, and I haven't found any obvious issues. LGTM. 👍
306bf35
to
48336f5
Compare
Add a table to store mint anchor transaction supply pre-commitment outputs for assets not issued by the local node. Data from this table will be used for supply commitment verification.
Adds support for upserting supply pre-commits that are not related to the minting process. These pre-commits pertain to assets issued by peer nodes.
Passes the proof type as an argument to universeUpsertProofLeaf rather than just its string representation. This refactor will simplify conditional logic based on proof type in a subsequent commit.
Extend universe leaf upsert with a method to conditionally upsert issuance supply pre-commitment records. This allows a node to record supply pre-commitments when syncing issuance proofs for assets it did not issue. These records are essential for verifying supply commitment transactions.
Introduce query to fetch unspent supply pre-commitment outputs. Each output originates from a mint anchor transaction and corresponds to an asset issuance where a peer node, not the local node, acted as the issuer. This differs from FetchUnspentMintSupplyPreCommits, which only returns pre-commitments minted by the local node.
48336f5
to
c9081c0
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.
@Roasbeef reviewed 26 of 26 files at r1, 1 of 3 files at r3, 23 of 23 files at r4, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @ffranr)
universe/supplyverifier/verifier.go
line 466 at r4 (raw file):
} if issuanceProof.Asset.GroupKey.GroupPubKey !=
Should this be instead IsEqual
?
Update method SupplyCommitMachine.UnspentPrecommits to return pre-commitment outputs generated by remote issuer nodes, in addition to those from the local node.
Now that supply pre-commitments are stored for remotely issued assets, we can assert in the supply verifier that if a supply commitment is the initial one (i.e., the spent commitment field is None), then the corresponding transaction must spend some pre-commitment outputs. This enforces a link between the initial supply commitment transaction and a mint anchor transaction.
c9081c0
to
e8ea035
Compare
I’ve updated the
|
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 👙
Closes #1782
This PR strengthens the supply commitment verification process by enforcing that initial supply commitments must spend pre-commitment outputs. This requirement ties the first supply commitment transaction directly to a mint anchor transaction.
Key changes:
Verification logic
supplyverifier
, added an assertion that initial supply commitments must spend pre-commitment outputs. This ties the first supply commitment transaction directly to a mint anchor transaction, ensuring the chain of authentication is enforced for both local and remote issuances.Database queries and schema
FetchUnspentMintSupplyPreCommits
) and those created by remote issuers (FetchUnspentSupplyPreCommits
).tapdb enhancements
SupplyCommitMachine.UnspentPrecommits
to return both local and remote issuer pre-commitments.This change is