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

[feature]: Append the listchannels command with the short_chan_id representation (XXXX:XX:X) besides the 8 byte integer one. #8650

Open
ziggie1984 opened this issue Apr 15, 2024 · 25 comments · May be fixed by #8676
Labels
channels enhancement Improvements to existing features / behaviour good first issue Issues suitable for first time contributors to LND P2 should be fixed if one has time

Comments

@ziggie1984
Copy link
Collaborator

Currently we only have the 8 byte representation in the listchannels output, but we have several parts in the logs where we would only log the short channel id in a different format (block:transaction:output) for an easier analysis it makes sense to also show this representation when querying the channels.

Moreover I think we can add the Channel ID as well, so we have all possible representations of the channel. This means we should depict the real chan_id in the listchannels output (chan_id : https://github.com/lightning/bolts/blob/master/02-peer-protocol.md#definition-of-channel_id) and two short_chan_id representations (integer64 and block:transaction:output) next to it.

@ziggie1984 ziggie1984 added enhancement Improvements to existing features / behaviour channels good first issue Issues suitable for first time contributors to LND labels Apr 15, 2024
@andiz2
Copy link

andiz2 commented Apr 15, 2024

Hi @ziggie1984 !

I can have a look at the listchannels and do the asked changes.
Would come back with updates soon 😄

@andiz2
Copy link

andiz2 commented Apr 17, 2024

At the moment struggleing to install the dependencies...
I run 'make install' after changed the go version in go.mod to 1.21
..One of them are:
chainntnfs/best_block_view.go:30:25: undefined: atomic.Pointer
chainntnfs/mempool.go:27:18: undefined: atomic.Uint64
note: module requires Go 1.21

google.golang.org/grpc

/home/beast/go/pkg/mod/google.golang.org/[email protected]/server.go:2082:14: undefined: atomic.Int64
note: module requires Go 1.19

github.com/lightningnetwork/lnd/lnwire

lnwire/accept_channel.go:269:33: cannot use fn.a (type tlv.RecordT[*github.com/lightningnetwork/lnd/tlv.tlvType4,"".Musig2Nonce]) as type go.shape.struct { github.com/lightningnetwork/lnd/tlv.recordType *uint8; Val [66]uint8 }_0 in assignment
lnwire/channel_ready.go:95:37: cannot use fn.a (type tlv.RecordT[*github.com/lightningnetwork/lnd/tlv.tlvType4,"".Musig2Nonce]) as type go.shape.struct { github.com/lightningnetwork/lnd/tlv.recordType *uint8; Val [66]uint8 }_0 in assignment
lnwire/channel_reestablish.go:217:33: cannot use fn.a (type tlv.RecordT[*github.com/lightningnetwork/lnd/tlv.tlvType4,"".Musig2Nonce]) as type go.shape.struct { github.com/lightningnetwork/lnd/tlv.recordType *uint8; Val [66]uint8 }_0 in assignment
lnwire/closing_complete.go:66:37: cannot use fn.a (type tlv.RecordT[*github.com/lightningnetwork/lnd/tlv.tlvType1,"".Sig]) as type go.shape.struct { github.com/lightningnetwork/lnd/tlv.recordType *uint8; Val struct { "".bytes [64]uint8; "".sigType "".sigType } }_0 in assignment
lnwire/closing_complete.go:69:37: cannot use fn.a (type tlv.RecordT[*github.com/lightningnetwork/lnd/tlv.tlvType2,"".Sig]) as type go.shape.struct { github.com/lightningnetwork/lnd/tlv.recordType *uint8; Val struct { "".bytes [64]uint8; "".sigType "".sigType } }_0 in assignment
lnwire/closing_complete.go:72:38: cannot use fn.a (type tlv.RecordT[*github.com/lightningnetwork/lnd/tlv.tlvType3,"".Sig]) as type go.shape.struct { github.com/lightningnetwork/lnd/tlv.recordType *uint8; Val struct { "".bytes [64]uint8; "".sigType "".sigType } }_0 in assignment
lnwire/closing_signed.go:87:33: cannot use fn.a (type tlv.RecordT[*github.com/lightningnetwork/lnd/tlv.tlvType6,"".PartialSig]) as type go.shape.struct { github.com/lightningnetwork/lnd/tlv.recordType *uint8; Val struct { Sig github.com/decred/dcrd/dcrec/secp256k1/v4.ModNScalar
} }_0 in assignment
lnwire/commit_sig.go:92:33: cannot use fn.a (type tlv.RecordT[*github.com/lightningnetwork/lnd/tlv.tlvType2,"".PartialSigWithNonce]) as type go.shape.struct { github.com/lightningnetwork/lnd/tlv.recordType *uint8; Val struct { "".PartialSig; Nonce "".Musig2Nonce } }_0 in assignment
lnwire/funding_created.go:103:33: cannot use fn.a (type tlv.RecordT[*github.com/lightningnetwork/lnd/tlv.tlvType2,"".PartialSigWithNonce]) as type go.shape.struct { github.com/lightningnetwork/lnd/tlv.recordType *uint8; Val struct { "".PartialSig; Nonce "".Musig2Nonce } }_0 in assignment
lnwire/funding_signed.go:89:33: cannot use fn.a (type tlv.RecordT[*github.com/lightningnetwork/lnd/tlv.tlvType2,"".PartialSigWithNonce]) as type go.shape.struct { github.com/lightningnetwork/lnd/tlv.recordType *uint8; Val struct { "".PartialSig; Nonce "".Musig2Nonce } }_0 in assignment
lnwire/funding_signed.go:89:33: too many errors
note: module requires Go 1.21

modernc.org/sqlite/lib

/home/beast/go/pkg/mod/modernc.org/[email protected]/lib/sqlite_linux_amd64.go:24762:51: undefined: libc.Xpread
note: module requires Go 1.20

Does anyone encountered some of these before?

@guggero
Copy link
Collaborator

guggero commented Apr 18, 2024

That sounds like you didn't install Go 1.21 or later correctly. Make sure you uninstalled any previous versions fully.

@andiz2
Copy link

andiz2 commented Apr 18, 2024

I will uninstall it and add it again.

@andiz2
Copy link

andiz2 commented Apr 18, 2024

sudo make install
[sudo] password for beast:
fatal: No names found, cannot describe anything.
/bin/sh: 1: go: not found
expr: syntax error: unexpected argument ‘21’
/bin/sh: 1: go: not found
Installing lnd and lncli.
go install -v -tags="" -ldflags=" -s -w -buildid= -X github.com/lightningnetwork/lnd/build.Commit=" github.com/lightningnetwork/lnd/cmd/lnd
/bin/sh: 1: go: not found
make: *** [Makefile:130: install-binaries] Error 127
beast@DESKTOP-TFSG0II:/mnt/c/Users/admin/Documents/code go/lighningNNNN/lnd$ go version
go version go1.21.9 linux/amd64
beast@DESKTOP-TFSG0II:/mnt/c/Users/admin/Documents/code go/lighningNNNN/lnd$

I've reinstalled correct version for go but I still have some issues..

@mohamedawnallah
Copy link
Contributor

The use of sudo typically resets environment variables for security reasons. This behavior can be adjusted with the env_keep directive in the sudoers file. Alternatively, you could try the following command to include the current PATH or the path where your go binary located:

sudo -E env "PATH=$PATH" make install

@andiz2
Copy link

andiz2 commented Apr 19, 2024

I've installed correctly now.
Ran tests and still got some tests failing
--- FAIL: TestLightningWalletBitcoindRPCPolling (13.20s)
--- FAIL: TestLightningWalletBitcoindRPCPolling/btcwallet/bitcoind-rpc-polling:change_output_spend_confirmation (0.05s)
test_interface.go:2222:
Error Trace: /mnt/c/Users/admin/Documents/code go/lighningNNNN/lnd/lnwallet/test/test_interface.go:175
/mnt/c/Users/admin/Documents/code go/lighningNNNN/lnd/lnwallet/test/test_interface.go:2222
/mnt/c/Users/admin/Documents/code go/lighningNNNN/lnd/lnwallet/test/test_interface.go:3378
Error: Received unexpected error:
undefined: -8: Fee rates larger than or equal to 1BTC/kvB are not accepted
Test: TestLightningWalletBitcoindRPCPolling/btcwallet/bitcoind-rpc-polling:change_output_spend_confirmation
Messages: unable to send transaction
--- FAIL: TestLightningWalletBitcoindZMQ (13.63s)
--- FAIL: TestLightningWalletBitcoindZMQ/btcwallet/bitcoind:change_output_spend_confirmation (0.03s)
test_interface.go:2222:
Error Trace: /mnt/c/Users/admin/Documents/code go/lighningNNNN/lnd/lnwallet/test/test_interface.go:175
/mnt/c/Users/admin/Documents/code go/lighningNNNN/lnd/lnwallet/test/test_interface.go:2222
/mnt/c/Users/admin/Documents/code go/lighningNNNN/lnd/lnwallet/test/test_interface.go:3378
Error: Received unexpected error:
undefined: -8: Fee rates larger than or equal to 1BTC/kvB are not accepted
Test: TestLightningWalletBitcoindZMQ/btcwallet/bitcoind:change_output_spend_confirmation
Messages: unable to send transaction
FAIL
FAIL github.com/lightningnetwork/lnd/lnwallet/test/bitcoind 13.664s
FAIL
--- FAIL: TestInvoiceRegistry (0.41s)
postgres_fixture.go:70:
Error Trace: /root/go/pkg/mod/github.com/lightningnetwork/lnd/[email protected]/postgres_fixture.go:70
/mnt/c/Users/admin/Documents/code go/lighningNNNN/lnd/invoices/invoiceregistry_test.go:121
Error: Received unexpected error:
API error (400): failed to resolve image name: short-name "postgres:11" did not resolve to an alias and no unqualified-search registries are defined in "/etc/containers/registries.conf"
Test: TestInvoiceRegistry
Messages: Could not start resource
--- FAIL: TestInvoices (0.00s)
postgres_fixture.go:70:
Error Trace: /root/go/pkg/mod/github.com/lightningnetwork/lnd/[email protected]/postgres_fixture.go:70
/mnt/c/Users/admin/Documents/code go/lighningNNNN/lnd/invoices/invoices_test.go:227
Error: Received unexpected error:
API error (400): failed to resolve image name: short-name "postgres:11" did not resolve to an alias and no unqualified-search registries are defined in "/etc/containers/registries.conf"
Test: TestInvoices
Messages: Could not start resource
FAIL
FAIL github.com/lightningnetwork/lnd/invoices 0.444s
make: *** [Makefile:221: unit] Error 123

I don't understand why looking for image/container since I didn't used

@guggero
Copy link
Collaborator

guggero commented Apr 19, 2024

Fee rates larger than or equal to 1BTC/kvB are not accepted

bitcoind check again after rebasing on current master, this was just fixed (or don't use bitcoind v27.0).

The Postgres image is used for some unit tests as a test database backend.

@andiz2
Copy link

andiz2 commented Apr 19, 2024

What should I use instead of this? 'or don't use bitcoind v27.0'

@guggero
Copy link
Collaborator

guggero commented Apr 19, 2024

An older version, e.g. v26.

@andiz2
Copy link

andiz2 commented Apr 19, 2024

Thats's good news! I will downgrade

@andiz2
Copy link

andiz2 commented Apr 19, 2024

-- FAIL: TestInvoiceRegistry (0.07s)
postgres_fixture.go:70:
Error Trace: /root/go/pkg/mod/github.com/lightningnetwork/lnd/[email protected]/postgres_fixture.go:70
/mnt/c/Users/admin/Documents/code go/lighningNNNN/lnd/invoices/invoiceregistry_test.go:121
Error: Received unexpected error:
API error (400): failed to resolve image name: short-name "postgres:11" did not resolve to an alias and no unqualified-search registries are defined in "/etc/containers/registries.conf"
Test: TestInvoiceRegistry
Messages: Could not start resource
--- FAIL: TestInvoices (0.00s)
postgres_fixture.go:70:
Error Trace: /root/go/pkg/mod/github.com/lightningnetwork/lnd/[email protected]/postgres_fixture.go:70
/mnt/c/Users/admin/Documents/code go/lighningNNNN/lnd/invoices/invoices_test.go:227
Error: Received unexpected error:
API error (400): failed to resolve image name: short-name "postgres:11" did not resolve to an alias and no unqualified-search registries are defined in "/etc/containers/registries.conf"
Test: TestInvoices
Messages: Could not start resource
FAIL
FAIL github.com/lightningnetwork/lnd/invoices 0.114s

make: *** [Makefile:221: unit] Error 123

After downgrade make test looks like this..is there any way to resolve these?

@mohamedawnallah
Copy link
Contributor

By the way, @andiz2, you could still contribute even if some of your test cases fail. They will run in the CI anyway. I still have some failed test cases locally, and some of them are flaky.

@andiz2
Copy link

andiz2 commented Apr 21, 2024

Looks more clear now!
I would add 2 new fileds in the lnrpc.Channel struct:
ShortChannelIdInt64 and ShortChannelIdTx
I would use these fields in

func createRPCOpenChannel(r *rpcServer, dbChannel *channeldb.OpenChannel,
	isActive, peerAliasLookup bool) (*lnrpc.Channel, error) {

and populate with proper values like these ones are

channel := &lnrpc.Channel{
		Active:                isActive,
		Private:               isPrivate(dbChannel),
		RemotePubkey:          nodeID,
		....

Then it will be good to go :)
Correct me if I am wrong

@mohamedawnallah
Copy link
Contributor

Yes and also update the release notes 👍

@alexbosworth
Copy link
Contributor

Another representation is height x index x vout like #2432

@ziggie1984
Copy link
Collaborator Author

Another representation is height x index x vout like #2432

Very good idea, I think using x instead of : makes sense because its seems to be the right format also mentioned in the BOLT Spec

Maybe a separate PR could implement this comment as well, so make sure we accept both formats in our RPC commands.
#2432 (comment)

@andiz2
Copy link

andiz2 commented Apr 22, 2024 via email

@ziggie1984
Copy link
Collaborator Author

Yes, use x instead of :, as long as we present both version of the short_chan_id (integer and height x index x vout) we are fine.

@andiz2
Copy link

andiz2 commented Apr 22, 2024

So from what I've noticed the new command will have this output
(chan_id x short_chan_id_integer64 x block x transaction x output)
please confirm

@andiz2
Copy link

andiz2 commented Apr 22, 2024

Regarding lnrpc.Channel I see

// The unique channel ID for the channel.
	ChanId uint64 `protobuf:"varint,2,opt,name=chan_id,json=chanId,proto3" json:"chan_id,omitempty"`

that is used as ShortChannelID from what i see in

func createRPCOpenChannel...
          dbScid := dbChannel.ShortChannelID
          ChanId:                dbScid.ToUint64(),

shouldn't this be used to store real channel_id as name suggests?

@saubyk saubyk added the P2 should be fixed if one has time label Apr 22, 2024
@andiz2
Copy link

andiz2 commented Apr 22, 2024

I've added these to lnrpc.Channel:

// ChannelID is a series of 32-bytes that uniquely identifies all channels
ChannelID ChannelID `protobuf:"bytes,37,opt,name=channel_id,json=channelId,proto3" json:"channel_id,omitempty"`
// The short channel id as a string with x between the block height and tx index
ShortChannelIdStringX string `protobuf:"bytes,38,opt,name=short_channel_id_string_x,json=shortChannelIdStringX,proto3" json:"short_channel_id_string_x,omitempty"` 
}

along with methods:

func (x *Channel) GetRealChanId() uint64 {
	if x != nil {
		return x.ChannelID
	}
	return 0
}

func (x *Channel) GetShortChannelIdStringX() string {
	if x != nil {
		return x.ShortChannelIdStringX
	}
	return ""
}

and these 2 lines in the channel init in method createRPCOpenChannel

ChannelID:             chanID,
ShortChannelIdStringX: dbScid.StringX(),

I saw that the short chan integer64 is already done via

ChanId:                dbScid.ToUint64(),

I will update if there was misunderstoods 😄

@ziggie1984
Copy link
Collaborator Author

When working on an issue, create a PR set it into draft and from there ask for questions when you are stuck. Otherwise it costs too much bandwidth for reviewers. Always bundle questions so that it's not a constant back and forth.
Thank you in advance.

andiz2 added a commit to andiz2/lnd that referenced this issue Apr 22, 2024
@andiz2
Copy link

andiz2 commented Apr 22, 2024

@ziggie1984 you might be right... Maybe I need to lurk around for some time until I have a deeper understanding of the project and then try to contribuite again...
I still have one curiosity if you can make me understand..
I saw that the short chan integer64 is already done via

ChanId:                dbScid.ToUint64(),

is this var ChanId or shortChanId as the name sugest it should be chanId..
Thanks in advance.

@ziggie1984
Copy link
Collaborator Author

Yes there is an inaccuracy see the description of the issue, we label the short_chan_id as chan_id in the current listchannels output and we should change that and depict the real chan_id here as well.

Moreover I think we can add the Channel ID as well, so we have all possible representations of the channel. This means we should depict the real chan_id in the listchannels output (chan_id : https://github.com/lightning/bolts/blob/master/02-peer-protocol.md#definition-of-channel_id) and two short_chan_id representations (integer64 and block:transaction:output) next to it.

Maybe I need to lurk around for some time until I have a deeper understanding of the project and then try to contribute again...

You learn the most by coding, so I would recommend to push through, learn to persist I am sure you will be able to solve this issue.

@saubyk saubyk linked a pull request Apr 22, 2024 that will close this issue
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
channels enhancement Improvements to existing features / behaviour good first issue Issues suitable for first time contributors to LND P2 should be fixed if one has time
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants