#418 libp2p integration #420

NhoxxKienn · 2025-04-25T10:35:13Z

Description

This PR solves the issue #418 of go-perun, which is part of the release v0.14.0 (#417). It migrates the wire implementation of Libp2p from perun-libp2p-wire directly into go-perun to be able to easily maintain in the future.

Location:

The implementation will be located in wire/net/libp2p

Features:

Libp2p's client wire implementation.
Unit tests
Integration test with go-perun's wire/net interface

Signed-off-by: Minh Huy Tran <huy@perun.network>

…al channel in some chains Signed-off-by: Minh Huy Tran <huy@perun.network>

…spute Signed-off-by: Minh Huy Tran <huy@perun.network>

Signed-off-by: Minh Huy Tran <huy@perun.network>

Copilot

Pull Request Overview

This PR migrates the Libp2p wire implementation into go‑perun and refactors related encoding, decoding, and testing functions to improve maintainability and integration. Key changes include incorporating new methods for balances and sub‑allocations, updating state types in the adjudicator, and removing duplicate or outdated functions.

Reviewed Changes

Copilot reviewed 164 out of 166 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
channel/allocation_test.go	Replaces traditional loop iterations with “for range” on integer literals
channel/allocation.go	Adds new balances methods and backend bounds checks; introduces an invalid loop in Sum
channel/adjudicator.go	Updates state mappings from State to SignedState
channel/actionmachine.go	Moves setStaging method without modifying behavior
backend/sim/wire/address.go	Introduces a new random address function and removes its duplicate
backend/sim/wire/account.go	Removes the duplicate NewRandomAccount implementation
backend/sim/wallet/wallet_test.go	Replaces assert with require for stricter error checking
backend/sim/wallet/wallet_internal_test.go	Switches loop iterations to “for range” on integer literals
backend/sim/wallet/wallet.go	Introduces documentation for the Wallet struct and removes duplicate code
backend/sim/wallet/address_internal_test.go	Uses “for range” on an int variable in test loops
backend/sim/wallet/address.go	Introduces a new NewRandomAddress (with minor formatting issues)
backend/sim/channel/asset_test.go	Updates loop iterations to use “for range” on integer literals
backend/sim/channel/asset.go	Adds bounds checks to asset unmarshalling and ensures asset ID validity
backend/sim/channel/app.go	Removes duplicate NewRandomAppID implementation
apps/payment/resolver_internal_test.go	Uses require instead of assert for error checking
apps/payment/randomizer_internal_test.go	Minimal adjustments on loop iterators
apps/payment/app_internal_test.go	Uses “for range” on an int where a traditional loop is expected
.github/workflows/ci.yml	Updates action versions for checkout, Go setup, and golangci-lint

Files not reviewed (2)

.golangci.json: Language not supported
.golangci.yml: Language not supported

Comments suppressed due to low confidence (5)

channel/allocation_test.go:166

Using 'for range 10' on an integer literal is not valid in Go. Please revert to a traditional for loop (e.g., for i := 0; i < 10; i++) to iterate the desired number of times.

for range 10 {

channel/allocation.go:309

Iterating with 'for i := range n' is invalid because 'n' is an integer. Consider iterating over the slice (e.g., for i := 0; i < n; i++) or using 'for i := range b' if b is the slice.

for i := range n {

backend/sim/wallet/wallet_internal_test.go:36

Using 'for range 10' on an integer literal is not valid in Go. Please use a traditional loop to iterate a fixed number of times.

for range 10 {

backend/sim/wallet/address_internal_test.go:53

Iterating with 'for i := range zeros' is invalid since 'zeros' is an integer. Use a conventional loop (e.g., for i := 0; i < zeros; i++) instead.

for i := range zeros {

apps/payment/app_internal_test.go:96

Looping with 'for i := range numParticipants' is invalid since numParticipants is an integer. Use a traditional loop such as 'for i := 0; i < numParticipants; i++' instead.

for i := range numParticipants {

Copilot · 2025-04-30T11:06:02Z

backend/sim/wallet/address.go

+func NewRandomAddress(rng io.Reader) *Address {
+	privateKey, err := ecdsa.GenerateKey(curve, rng)
+	if err != nil {
+		log.Panicf("Creation of account failed with error", err)


The Panicf call lacks a formatting verb for the error; consider using 'panicf("Creation of account failed: %v", err)' to properly include the error message.

Suggested change

log.Panicf("Creation of account failed with error", err)

log.Panicf("Creation of account failed: %v", err)

DragonDev1906

Feedback for future PRs:
Try to put changes like linter updates in a separate PR, especially when they need a lot of changes or want to move code around. Reviewing a PR is significantly easier if it is smaller and does one thing. When you need to update the linter during another change I'd suggest to temporarily disable the linters that require many changes (for example the one about the order of functions in a file or the integer overflow one) and fix them in a separate PR. That keeps the actual changes separate from changes that don't do much.

I have not looked too deep into wire/net/libp2p, as if I understood it correctly it was copied from another repository and (probably) only has smaller changes (e.g. for the linter).

As hinted at in the comments: There are some changes I'd put into a separate PR, as they are not about the libp2p integration but a more general issue I've noticed during review.

DragonDev1906 · 2025-05-30T09:01:52Z

apps/payment/app_internal_test.go

@@ -42,7 +42,7 @@ func TestApp_ValidInit(t *testing.T) {
 	assert.Panics(func() { app.ValidInit(nil, wrongdata) }) //nolint:errcheck

 	data := &channel.State{Data: Data()}
-	assert.Nil(app.ValidInit(nil, data))
+	assert.NoError(app.ValidInit(nil, data))


Shouldn't this be require.NoError?
assert.NoError doesn't panic but just returns a bool that isn't checked here.
https://github.com/stretchr/testify?tab=readme-ov-file#require-package

From a quick search it looks like we have that in a lot of places.
Recommendation/Suggestion: Keep it as it is now for this PR (bug existed before, too and this one is already big) and fix all of these in a separate PR.

DragonDev1906 · 2025-05-30T09:27:23Z

backend/sim/channel/asset.go

@@ -44,16 +46,23 @@ func NewRandomAsset(rng *rand.Rand) *Asset {
 // MarshalBinary marshals the address into its binary representation.
 func (a Asset) MarshalBinary() ([]byte, error) {
 	data := make([]byte, assetLen)
+	if a.ID < 0 {


Is there a reason that Asset.ID is int64 instead of uint64?

If not I'd suggest to make it a uint64 (and potentially even a private field). That way we don't need any of this integer casting.

The only external user I could find was the Test_Asset_GenericMarshaler test and that only generates positive IDs, so the code for a.ID < 0 isn't tested either:

// Int63 returns a non-negative pseudo-random 63-bit integer as an int64. func (r *Rand) Int63() int64 { return r.src.Int63() }

DragonDev1906 · 2025-05-30T09:34:29Z

backend/sim/wallet/address.go

+	return &Address{
+		Curve: privateKey.Curve,
+		X:     privateKey.X,
+		Y:     privateKey.Y,
+	}


Can probably be simplified:

Suggested change

return &Address{

Curve: privateKey.Curve,

X: privateKey.X,

Y: privateKey.Y,

}

return &privateKey.PublicKey

DragonDev1906 · 2025-05-30T10:13:17Z

channel/allocation.go

+	if assetLen > math.MaxUint16 {
+		return errors.New("too many assets")
+	}
+	balanceLen := len(a.Balances[0])


Is there an invariant (always-true statement) about Allocation that there is always at least one asset and thus at least one entry in balances? If so: I couldn't find it and this code would break for NewAllocation(2, backends) (0 assets).

We probably don't need to change anything here, but if there is/we want such an invariant we should probably enforce it in NewAllocation and document it somewhere.

DragonDev1906 · 2025-05-30T10:38:54Z

channel/persistence/keyvalue/cache.go

@@ -31,12 +31,15 @@ func newChannelCache() *channelCache {
 }

 // channelCache contains all channels.
+//
+//nolint:unused
 type channelCache struct {


Do we plan to use that at some point in the future or is this just dead code that should be removed?
This nolint was added 5 years ago: 50896f7

DragonDev1906 · 2025-05-30T11:28:52Z

client/test/virtualchannel.go

@@ -162,7 +222,7 @@ type VirtualChannelBalances struct {
 	FinalBalsBob        []*big.Int
 }

-func setupVirtualChannelTest(
+func setupVirtualChannelTest( //nolint


We probably shouldn't disable the entire linter but only those checks that are needed. That way it's also easier to see why the nolint is needed.

DragonDev1906 · 2025-05-30T11:30:59Z

client/test/virtualchannel.go

@@ -213,10 +274,10 @@ func setupVirtualChannelTest(
 		}
 	}
 	var updateProposalHandlerIngrid client.UpdateHandlerFunc = func(
-		s *channel.State, cu client.ChannelUpdate, ur *client.UpdateResponder,
+		_ *channel.State, cu client.ChannelUpdate, ur *client.UpdateResponder,


None of these are used. We should therefore (for consistency) use _ for all of them, instead of just the one with a single-letter-name.

DragonDev1906 · 2025-05-30T11:33:20Z

client/test/virtualchannel.go

@@ -280,14 +341,40 @@ func setupVirtualChannelTest(
 		}
 	}
 	var updateProposalHandlerBob client.UpdateHandlerFunc = func(
-		s *channel.State, cu client.ChannelUpdate, ur *client.UpdateResponder,
+		_ *channel.State, cu client.ChannelUpdate, ur *client.UpdateResponder,


Here, too: cu is unused and should probably be called _, too.

DragonDev1906 · 2025-05-30T11:43:11Z

wallet/address_test.go

@@ -119,7 +119,7 @@ func TestCloneAddresses(t *testing.T) {
 	addrs := wallettest.NewRandomAddressArray(rng, 3, TestBackendID)
 	addrs0 := wallet.CloneAddresses(addrs)
 	require.Equal(t, addrs, addrs0)
-	require.NotSame(t, addrs, addrs0)
+	require.NotSame(t, &addrs, &addrs0)


Unless I'm mistaken, this check becomes useless with two &, as you're now checking if the two variables of the stack have the same address (instead of whether the slices point to the same address. ¹

Footnotes

Though I'm not sure if the previous one without & did what was intended either. ↩

DragonDev1906 · 2025-05-30T12:03:45Z

wire/net/test/connhub_internal_test.go

@@ -51,7 +51,7 @@ func TestConnHub_Create(t *testing.T) {
 			ct.Stage("accept", func(rt pkgtest.ConcT) {
 				conn, err := l.Accept(ser)
 				assert.NoError(err)
-				require.NotNil(rt, conn)
+				assert.NotNil(rt, conn)


As mentioned above: Don't replace require with assert (which returns a bool instead of panicking).

NhoxxKienn added 14 commits January 23, 2025 06:05

feat: add aux attribute in channel.Params

3d65852

Signed-off-by: Minh Huy Tran <huy@perun.network>

feat(adjudicate): gather state map switchs to gather signed states

2bb1a55

Signed-off-by: Minh Huy Tran <huy@perun.network>

feat(vc_test): add aux params to virtual channel test for utxo chains

8c7df65

Signed-off-by: Minh Huy Tran <huy@perun.network>

feat(vc test): send parent ID in aux instead for utxo chains

e363289

Signed-off-by: Minh Huy Tran <huy@perun.network>

fix(vc test): fix aux generation in virtual channel test

0edd2b7

Signed-off-by: Minh Huy Tran <huy@perun.network>

feat(test/virtualchannel): add wait time for processing opening virtu…

634091b

…al channel in some chains Signed-off-by: Minh Huy Tran <huy@perun.network>

feat: add log and proper secondary settle for virtual channel test di…

1eb5cc3

…spute Signed-off-by: Minh Huy Tran <huy@perun.network>

feat: add additional payment channel test setup

61bf760

Signed-off-by: Minh Huy Tran <huy@perun.network>

feat: add proper clean up for virtual channel optimistic test

15bf682

Signed-off-by: Minh Huy Tran <huy@perun.network>

feat: add client's handler for virtual channel test cleanup

4d68d28

Signed-off-by: Minh Huy Tran <huy@perun.network>

Merge branch 'main' into nervos_ckb

17ac78d

refactor(lint): update and refactor golangci-lint

22cdc68

Signed-off-by: Minh Huy Tran <huy@perun.network>

feat(wire/net/libp2p): add initial package libp2p

4eb64e3

Signed-off-by: Minh Huy Tran <huy@perun.network>

feat(go.mod): update go dependencies

fddc7a9

Signed-off-by: Minh Huy Tran <huy@perun.network>

hyperledger-labs deleted a comment from mergify bot Apr 28, 2025

NhoxxKienn added 4 commits April 28, 2025 14:43

feat(libp2p): add libp2p account, address

eff95d6

Signed-off-by: Minh Huy Tran <huy@perun.network>

feat(libp2p): add dialer and listener

848f107

Signed-off-by: Minh Huy Tran <huy@perun.network>

feat(libp2p): add exchange addr tests

effbd00

Signed-off-by: Minh Huy Tran <huy@perun.network>

feat(wire/net): add bus test for libp2p and simple

f5bde07

Signed-off-by: Minh Huy Tran <huy@perun.network>

NhoxxKienn requested review from Copilot and iljabvh April 30, 2025 11:05

Copilot AI reviewed Apr 30, 2025

View reviewed changes

NhoxxKienn linked an issue May 14, 2025 that may be closed by this pull request

🧩 Libp2p in go-perun #418

Open

4 tasks

Merge branch 'main' into 418_libp2p_integration

14d52e5

iljabvh requested a review from DragonDev1906 May 30, 2025 08:24

DragonDev1906 reviewed May 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

#418 libp2p integration #420

#418 libp2p integration #420

Uh oh!

NhoxxKienn commented Apr 25, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 30, 2025

Uh oh!

DragonDev1906 left a comment

Uh oh!

DragonDev1906 May 30, 2025

Uh oh!

DragonDev1906 May 30, 2025

Uh oh!

DragonDev1906 May 30, 2025

Uh oh!

DragonDev1906 May 30, 2025

Uh oh!

DragonDev1906 May 30, 2025

Uh oh!

DragonDev1906 May 30, 2025

Uh oh!

DragonDev1906 May 30, 2025

Uh oh!

DragonDev1906 May 30, 2025

Uh oh!

DragonDev1906 May 30, 2025

Uh oh!

DragonDev1906 May 30, 2025

Uh oh!

Uh oh!

	log.Panicf("Creation of account failed with error", err)
	log.Panicf("Creation of account failed: %v", err)

#418 libp2p integration #420

Are you sure you want to change the base?

#418 libp2p integration #420

Uh oh!

Conversation

NhoxxKienn commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Location:

Features:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

DragonDev1906 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NhoxxKienn commented Apr 25, 2025 •

edited

Loading