From fe8784aa0cd66183214cc1cca5a4017aa8b376ad Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 25 Apr 2024 16:30:47 -0700 Subject: [PATCH] channeldb: fix race in TestPackager by removing global test var In this commit, we fix a race in the `TestPackager` series on channeldb. A few tests were sharing the same global variable of the set of log updates, which includes a pointer to an HTLC. The `ExtraData` value of the HTLC would then be mutated once we go to encode the message on disk. To fix this, we the global with a function that returns a new instance of all the test data. ``` ================== WARNING: DATA RACE Write at 0x0000021b0a48 by goroutine 2896: github.com/lightningnetwork/lnd/lnwire.(*ExtraOpaqueData).PackRecords() /home/runner/work/lnd/lnd/lnwire/extra_bytes.go:74 +0x546 github.com/lightningnetwork/lnd/lnwire.EncodeMessageExtraData() /home/runner/work/lnd/lnd/lnwire/extra_bytes.go:121 +0x4d github.com/lightningnetwork/lnd/lnwire.(*UpdateAddHTLC).Encode() /home/runner/work/lnd/lnd/lnwire/update_add_htlc.go:164 +0x5af github.com/lightningnetwork/lnd/lnwire.WriteMessage() /home/runner/work/lnd/lnd/lnwire/message.go:330 +0x351 github.com/lightningnetwork/lnd/channeldb.WriteElement() /home/runner/work/lnd/lnd/channeldb/codec.go:186 +0x1975 github.com/lightningnetwork/lnd/channeldb.WriteElements() /home/runner/work/lnd/lnd/channeldb/codec.go:247 +0x14f github.com/lightningnetwork/lnd/channeldb.serializeLogUpdate() /home/runner/work/lnd/lnd/channeldb/channel.go:2529 +0x3c github.com/lightningnetwork/lnd/channeldb.putLogUpdate() /home/runner/work/lnd/lnd/channeldb/forwarding_package.go:525 +0xae github.com/lightningnetwork/lnd/channeldb.(*ChannelPackager).AddFwdPkg() /home/runner/work/lnd/lnd/channeldb/forwarding_package.go:489 +0x684 github.com/lightningnetwork/lnd/channeldb_test.TestPackagerOnlyAdds.func1() /home/runner/work/lnd/lnd/channeldb/forwarding_package_test.go:283 +0x4c github.com/btcsuite/btcwallet/walletdb/bdb.(*db).Update() /home/runner/go/pkg/mod/github.com/btcsuite/btcwallet/walletdb@v1.4.2/bdb/db.go:429 +0xe5 github.com/lightningnetwork/lnd/kvdb.Update() /home/runner/go/pkg/mod/github.com/lightningnetwork/lnd/kvdb@v1.4.6/interface.go:16 +0x258 github.com/lightningnetwork/lnd/channeldb_test.TestPackagerOnlyAdds() /home/runner/work/lnd/lnd/channeldb/forwarding_package_test.go:282 +0x17b testing.tRunner() /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.21.4.linux-amd64/src/testing/testing.go:1595 +0x238 testing.(*T).Run.func1() /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.21.4.linux-amd64/src/testing/testing.go:1648 +0x44 Previous write at 0x0000021b0a48 by goroutine 2898: github.com/lightningnetwork/lnd/lnwire.(*ExtraOpaqueData).PackRecords() /home/runner/work/lnd/lnd/lnwire/extra_bytes.go:74 +0x546 github.com/lightningnetwork/lnd/lnwire.EncodeMessageExtraData() /home/runner/work/lnd/lnd/lnwire/extra_bytes.go:121 +0x4d github.com/lightningnetwork/lnd/lnwire.(*UpdateAddHTLC).Encode() /home/runner/work/lnd/lnd/lnwire/update_add_htlc.go:164 +0x5af github.com/lightningnetwork/lnd/lnwire.WriteMessage() /home/runner/work/lnd/lnd/lnwire/message.go:330 +0x351 github.com/lightningnetwork/lnd/channeldb.WriteElement() /home/runner/work/lnd/lnd/channeldb/codec.go:186 +0x1975 github.com/lightningnetwork/lnd/channeldb.WriteElements() /home/runner/work/lnd/lnd/channeldb/codec.go:247 +0x14f github.com/lightningnetwork/lnd/channeldb.serializeLogUpdate() /home/runner/work/lnd/lnd/channeldb/channel.go:2529 +0x3c github.com/lightningnetwork/lnd/channeldb.putLogUpdate() /home/runner/work/lnd/lnd/channeldb/forwarding_package.go:525 +0xae github.com/lightningnetwork/lnd/channeldb.(*ChannelPackager).AddFwdPkg() /home/runner/work/lnd/lnd/channeldb/forwarding_package.go:489 +0x684 github.com/lightningnetwork/lnd/channeldb_test.TestPackagerAddsThenSettleFails.func1() /home/runner/work/lnd/lnd/channeldb/forwarding_package_test.go:490 +0x4c github.com/btcsuite/btcwallet/walletdb/bdb.(*db).Update() /home/runner/go/pkg/mod/github.com/btcsuite/btcwallet/walletdb@v1.4.2/bdb/db.go:429 +0xe5 github.com/lightningnetwork/lnd/kvdb.Update() /home/runner/go/pkg/mod/github.com/lightningnetwork/lnd/kvdb@v1.4.6/interface.go:16 +0x2cd github.com/lightningnetwork/lnd/channeldb_test.TestPackagerAddsThenSettleFails() /home/runner/work/lnd/lnd/channeldb/forwarding_package_test.go:489 +0x1e7 testing.tRunner() /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.21.4.linux-amd64/src/testing/testing.go:1595 +0x238 testing.(*T).Run.func1() /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.21.4.linux-amd64/src/testing/testing.go:1648 +0x44 Goroutine 2896 (running) created at: testing.(*T).Run() /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.21.4.linux-amd64/src/testing/testing.go:1648 +0x82a testing.runTests.func1() /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.21.4.linux-amd64/src/testing/testing.go:2054 +0x84 testing.tRunner() /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.21.4.linux-amd64/src/testing/testing.go:1595 +0x238 testing.runTests() /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.21.4.linux-amd64/src/testing/testing.go:2052 +0x896 testing.(*M).Run() /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.21.4.linux-amd64/src/testing/testing.go:1925 +0xb57 github.com/lightningnetwork/lnd/kvdb.RunTests() /home/runner/go/pkg/mod/github.com/lightningnetwork/lnd/kvdb@v1.4.6/test_utils.go:23 +0x26 github.com/lightningnetwork/lnd/channeldb.TestMain() /home/runner/work/lnd/lnd/channeldb/setup_test.go:10 +0x308 main.main() _testmain.go:321 +0x303 Goroutine 2898 (running) created at: testing.(*T).Run() /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.21.4.linux-amd64/src/testing/testing.go:1648 +0x82a testing.runTests.func1() /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.21.4.linux-amd64/src/testing/testing.go:2054 +0x84 testing.tRunner() /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.21.4.linux-amd64/src/testing/testing.go:1595 +0x238 testing.runTests() /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.21.4.linux-amd64/src/testing/testing.go:2052 +0x896 testing.(*M).Run() /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.21.4.linux-amd64/src/testing/testing.go:1925 +0xb57 github.com/lightningnetwork/lnd/kvdb.RunTests() /home/runner/go/pkg/mod/github.com/lightningnetwork/lnd/kvdb@v1.4.6/test_utils.go:23 +0x26 github.com/lightningnetwork/lnd/channeldb.TestMain() /home/runner/work/lnd/lnd/channeldb/setup_test.go:10 +0x308 main.main() _testmain.go:321 +0x303 ================== ``` --- channeldb/forwarding_package_test.go | 55 +++++++++++++++++----------- 1 file changed, 34 insertions(+), 21 deletions(-) diff --git a/channeldb/forwarding_package_test.go b/channeldb/forwarding_package_test.go index d796c6d51..c113d13d5 100644 --- a/channeldb/forwarding_package_test.go +++ b/channeldb/forwarding_package_test.go @@ -142,8 +142,31 @@ func checkPkgFilterEncodeDecode(t *testing.T, i uint16, f *channeldb.PkgFilter) var ( chanID = lnwire.NewChanIDFromOutPoint(wire.OutPoint{}) +) - adds = []channeldb.LogUpdate{ +func testSettleFails() []channeldb.LogUpdate { + return []channeldb.LogUpdate{ + { + LogIndex: 2, + UpdateMsg: &lnwire.UpdateFulfillHTLC{ + ChanID: chanID, + ID: 0, + PaymentPreimage: [32]byte{0}, + }, + }, + { + LogIndex: 3, + UpdateMsg: &lnwire.UpdateFailHTLC{ + ChanID: chanID, + ID: 1, + Reason: []byte{}, + }, + }, + } +} + +func testAdds() []channeldb.LogUpdate { + return []channeldb.LogUpdate{ { LogIndex: 0, UpdateMsg: &lnwire.UpdateAddHTLC{ @@ -165,26 +188,7 @@ var ( }, }, } - - settleFails = []channeldb.LogUpdate{ - { - LogIndex: 2, - UpdateMsg: &lnwire.UpdateFulfillHTLC{ - ChanID: chanID, - ID: 0, - PaymentPreimage: [32]byte{0}, - }, - }, - { - LogIndex: 3, - UpdateMsg: &lnwire.UpdateFailHTLC{ - ChanID: chanID, - ID: 1, - Reason: []byte{}, - }, - }, - } -) +} // TestPackagerEmptyFwdPkg checks that the state transitions exhibited by a // forwarding package that contains no adds, fails or settles. We expect that @@ -273,6 +277,8 @@ func TestPackagerOnlyAdds(t *testing.T) { t.Fatalf("no forwarding packages should exist, found %d", len(fwdPkgs)) } + adds := testAdds() + // Next, create and write a new forwarding package that only has add // htlcs. fwdPkg := channeldb.NewFwdPkg(shortChanID, 0, adds, nil) @@ -377,6 +383,7 @@ func TestPackagerOnlySettleFails(t *testing.T) { // Next, create and write a new forwarding package that only has add // htlcs. + settleFails := testSettleFails() fwdPkg := channeldb.NewFwdPkg(shortChanID, 0, nil, settleFails) nSettleFails := len(settleFails) @@ -479,8 +486,11 @@ func TestPackagerAddsThenSettleFails(t *testing.T) { t.Fatalf("no forwarding packages should exist, found %d", len(fwdPkgs)) } + adds := testAdds() + // Next, create and write a new forwarding package that only has add // htlcs. + settleFails := testSettleFails() fwdPkg := channeldb.NewFwdPkg(shortChanID, 0, adds, settleFails) nAdds := len(adds) @@ -612,8 +622,11 @@ func TestPackagerSettleFailsThenAdds(t *testing.T) { t.Fatalf("no forwarding packages should exist, found %d", len(fwdPkgs)) } + adds := testAdds() + // Next, create and write a new forwarding package that has both add // and settle/fail htlcs. + settleFails := testSettleFails() fwdPkg := channeldb.NewFwdPkg(shortChanID, 0, adds, settleFails) nAdds := len(adds)