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

build: update to kvdb v1.4.3 #7960

Merged
merged 3 commits into from
Sep 12, 2023
Merged

Conversation

Roasbeef
Copy link
Member

@Roasbeef Roasbeef commented Sep 5, 2023

We needed a tag after updating the module with retry for sqlite/postgres.

@Roasbeef Roasbeef force-pushed the update-kv-db branch 2 times, most recently from 470cbd6 to 0fb03f7 Compare September 6, 2023 03:09
Copy link
Collaborator

@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.

LGTM 🎉

Would be nice to bump the other gomodules in this PR as well to make sure we don't need yet another PR with changes.

go.mod Show resolved Hide resolved
kvdb/sqlbase/sqlerrors_no_postgres.go Outdated Show resolved Hide resolved
kvdb/sqlbase/sqlerrors_sqlite.go Show resolved Hide resolved
Copy link
Member

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

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

Need to remove replace github.com/lightningnetwork/lnd/kvdb => ./kvdb otherwise good to go🙏

@Roasbeef
Copy link
Member Author

Roasbeef commented Sep 6, 2023

Need to remove replace github.com/lightningnetwork/lnd/kvdb => ./kvdb otherwise good to go🙏

Can't remove this, since I had to modify the build tags in kvdb package. Or I can do it in another PR, tag the version, then update this.

Otherwise plan was: merge this, tag, then new PR to do rc3 and update the module.

@Roasbeef Roasbeef force-pushed the update-kv-db branch 3 times, most recently from 05b22b6 to 823e8fd Compare September 7, 2023 02:22
Copy link
Collaborator

@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.

Needs one more tweak to get the itests passing, then we're good to go:

diff --git a/itest/lnd_channel_funding_utxo_selection_test.go b/itest/lnd_channel_funding_utxo_selection_test.go
index f52af7a41..27c831351 100644
--- a/itest/lnd_channel_funding_utxo_selection_test.go
+++ b/itest/lnd_channel_funding_utxo_selection_test.go
@@ -91,7 +91,7 @@ func testChannelUtxoSelection(ht *lntest.HarnessTest) {
                        feeRate:            15,
                        expectedErrStr: "output amount(0.00000174 BTC) after " +
                                "subtracting fees(0.00001826 BTC) below dust " +
-                               "limit(0.0000033 BTC)",
+                               "limit(0.00000330 BTC)",
                },
                // Selected coins don't cover the minimum channel size.
                {
@@ -102,7 +102,7 @@ func testChannelUtxoSelection(ht *lntest.HarnessTest) {
                        feeRate:            1,
                        chanOpenShouldFail: true,
                        expectedErrStr: "available funds(0.00017877 BTC) " +
-                               "below the minimum amount(0.0002 BTC)",
+                               "below the minimum amount(0.00020000 BTC)",
                },
                // The local amount exceeds the value of the selected coins.
                {
@@ -114,7 +114,7 @@ func testChannelUtxoSelection(ht *lntest.HarnessTest) {
                        chanOpenShouldFail: true,
                        expectedErrStr: "not enough witness outputs to " +
                                "create funding transaction, need 0.00210337 " +
-                               "BTC only have 0.001 BTC available",
+                               "BTC only have 0.00100000 BTC available",
                },
                // We are spending two selected coins partially out of three
                // available in the wallet and expect a change output and the

Copy link
Member

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

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

Pending @guggero 's comments, then this is good to go🏖

Otherwise plan was: merge this, tag, then new PR to do rc3 and update the module

This works. Just need to follow up closely as otherwise the linter will fail for other rebased PRs.

We needed a tag after updating the module with retry for
sqlite/postgres.
In this commit, we use exhaustive build tags to ensure that we can
always build the `sqlbase` package, independent of the set build tags.
To do this, we move the type declarations _into_ the parsing functions.
This then allows us to create two versions for each db: with the db, and
without it.

To avoid a module tag round trip to get this working, we use a local
replace for now. Once this is merged in, we can do the tag (along side
rc3), then remove the replace.
@yyforyongyu
Copy link
Member

Unit tests for both the SQL backends failed, are those flakes?

@Roasbeef
Copy link
Member Author

@yyforyongyu I think so, just ran then locally and it passed.

@Roasbeef Roasbeef merged commit c77192a into lightningnetwork:master Sep 12, 2023
20 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants