-
Notifications
You must be signed in to change notification settings - Fork 123
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
Federation envoy re-attempts sync push for issuance proofs #698
Conversation
86a75d3
to
425e29a
Compare
425e29a
to
83393fa
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.
Did a first pass, looks pretty good so far. Nice work!
tapdb/sqlc/migrations/000012_universe_fed_proof_sync_log.up.sql
Outdated
Show resolved
Hide resolved
3b99d2a
to
cbf8d04
Compare
I've just added an itest to this PR. I think it's at a point now where I just need unit tests for the new SQL queries/db store methods. I think, to keep this PR simple, I'll add the functionality (and tests) to cleanup stale entries from the proof sync log in a separate PR. I'm confident that this PR will add the necessary columns to the new log table to make that possible. |
cbf8d04
to
7b3112e
Compare
I think this would be a good idea where sql query unit testing is concerned: package tapdb
import (
"database/sql"
"testing"
"time"
"github.com/lightninglabs/taproot-assets/tapdb/sqlc"
"github.com/lightningnetwork/lnd/clock"
)
// testDbStores is a helper struct that contains all the database stores that
// are needed for testing.
type testDbStores struct {
FedDb *UniverseFederationDB
MintingDb *AssetMintingStore
AssetDb *AssetStore
Db sqlc.Querier
}
// newTestDbStores creates a new set of test database stores.
func newTestDbStores(t *testing.T) testDbStores {
// Create a new test database.
db := NewTestDB(t)
testClock := clock.NewTestClock(time.Now())
// Gain a handle to the pending (minting) universe federation store.
universeServerTxCreator := NewTransactionExecutor(
db, func(tx *sql.Tx) UniverseServerStore {
return db.WithTx(tx)
},
)
fedStore := NewUniverseFederationDB(universeServerTxCreator, testClock)
// Gain a handle to the pending (minting) assets store.
assetMintingDB := NewTransactionExecutor(
db, func(tx *sql.Tx) PendingAssetStore {
return db.WithTx(tx)
},
)
assetMintingStore := NewAssetMintingStore(assetMintingDB)
// Gain a handle to the active assets store.
assetsDB := NewTransactionExecutor(
db, func(tx *sql.Tx) ActiveAssetsStore {
return db.WithTx(tx)
},
)
activeAssetsStore := NewAssetStore(assetsDB, testClock)
return testDbStores{
FedDb: fedStore,
MintingDb: assetMintingStore,
AssetDb: activeAssetsStore,
Db: db,
}
} And then we can have access to each store for the same db in any unit test. And we wont need to extend functions like |
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.
Very nice unit test addition!
I have one concern about performance, the rest of the comments are mostly nits or suggestions.
34dc0cd
to
b80924f
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.
Two small things that we should fix, otherwise LGTM 🎉
b80924f
to
1289f30
Compare
1289f30
to
d850e5e
Compare
This commit renames `ListUniverseServers` into `QueryUniverseServers`. It also adds a `WHERE` clause to the SQL statement to allow us to filter on server host or row ID.
This commit adds a new function called `NewUniIDFromRawArgs`. The function can be used to derive a universe identifier from raw asset ID/group key bytes. We will use this function to derive a universe ID from data stored in the database.
This commit also adds log query/upserts SQL statements.
This commit adds a flag to the federation universe push request to indicate that the proof leaf sync attempt should be logged and actively managed to ensure that the federation push procedure is repeated in the event of a failure.
d850e5e
to
ccb720a
Compare
This commit adds an integration test which helps to ensure that a minting node will retry pushing a minting proof to a federation server peer node, in the event that that peer node failed to receive the proof at the time of the initial sync attempt.
This structure will allow us to pass around the same db store handler to different helper functions which will aid in setting up a unit test db.
This commit adds a unit test db handler helper method which we will use in a new test (in a subsequent commit) to populate a unit test db with an asset and its corresponding proof.
This commit adds a helper method for inserting server addresses into a unit test db. It also adds a helper method for inserting a proof leaf into a unit test db. These helper methods will be used in a subsequent commit.
ccb720a
to
0cf92e0
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, some final nits on the unit test changes.
0cf92e0
to
a34ec30
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, solid PR!
Closes #526
This PR adds a new SQL table which logs the federation sync status of select proofs. In this PR we use the new table to log and re-attempt failed asset issuance proof push sync events.
The federation envoy is modified such that tick events kick-off syncing for pending proofs found in the new log table.
I would be happy to receive reviews to double check that I'm on the right track with these changes.
This PR is currently missing tests.