Skip to content
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

Merged
merged 16 commits into from
Dec 8, 2023

Conversation

ffranr
Copy link
Contributor

@ffranr ffranr commented Nov 27, 2023

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.

@ffranr ffranr changed the title Fed envoy push Federation envoy re-attempts sync push for issuance proofs Nov 27, 2023
@ffranr ffranr requested a review from jharveyb November 27, 2023 15:27
@ffranr ffranr force-pushed the fed-envoy-push branch 2 times, most recently from 86a75d3 to 425e29a Compare November 27, 2023 15:35
@dstadulis dstadulis added this to the v0.4 milestone Nov 27, 2023
Copy link
Member

@guggero guggero left a 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!

universe/interface.go Outdated Show resolved Hide resolved
universe/interface.go Outdated Show resolved Hide resolved
tapdb/sqlc/queries/universe.sql Show resolved Hide resolved
universe/auto_syncer.go Outdated Show resolved Hide resolved
universe/auto_syncer.go Outdated Show resolved Hide resolved
universe/auto_syncer.go Outdated Show resolved Hide resolved
@ffranr ffranr force-pushed the fed-envoy-push branch 2 times, most recently from 3b99d2a to cbf8d04 Compare November 29, 2023 17:28
@ffranr
Copy link
Contributor Author

ffranr commented Nov 29, 2023

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.

@ffranr
Copy link
Contributor Author

ffranr commented Nov 30, 2023

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 newTestFederationDb at each call site just to add a store that we need.

@ffranr ffranr marked this pull request as ready for review December 1, 2023 18:26
@ffranr ffranr requested review from guggero and GeorgeTsagk and removed request for Roasbeef and jharveyb December 1, 2023 18:26
Copy link
Member

@guggero guggero left a 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.

tapdb/universe_federation.go Show resolved Hide resolved
tapdb/universe_federation.go Show resolved Hide resolved
tapdb/sqlc/queries/universe.sql Outdated Show resolved Hide resolved
universe/auto_syncer.go Outdated Show resolved Hide resolved
tapdb/universe_federation.go Outdated Show resolved Hide resolved
tapdb/sqlc/queries/universe.sql Outdated Show resolved Hide resolved
tapdb/test_sqlutils.go Outdated Show resolved Hide resolved
tapdb/assets_store_test.go Outdated Show resolved Hide resolved
tapdb/universe_federation_test.go Outdated Show resolved Hide resolved
tapdb/universe_federation_test.go Outdated Show resolved Hide resolved
Copy link
Member

@guggero guggero left a 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 🎉

tapdb/universe_test.go Outdated Show resolved Hide resolved
itest/universe_federation_test.go Outdated Show resolved Hide resolved
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.
@ffranr ffranr requested review from jharveyb and removed request for GeorgeTsagk December 8, 2023 12:41
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.
Copy link
Collaborator

@jharveyb jharveyb left a 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.

tapdb/universe_federation_test.go Outdated Show resolved Hide resolved
tapdb/universe_federation_test.go Show resolved Hide resolved
tapdb/universe_federation_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@jharveyb jharveyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, solid PR!

@guggero guggero added this pull request to the merge queue Dec 8, 2023
Merged via the queue into main with commit 6b39ff8 Dec 8, 2023
14 checks passed
@guggero guggero deleted the fed-envoy-push branch January 8, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

universe: add background loop for optimistic asset proof to FederationEnvoy
4 participants