[wallet 1/3]: refactor in preparation for group key channel funding

[guggero]

This is a pure refactor PR that prepares some of the wallet code to better deal with funding grouped asset packets.
We also add unit test coverage for some of the funding logic.

---

This change is 

Part of the group key support saga:

• Part 1: This PR
• Part 2: [wallet 2/3]: refactor funding logic to use allocation code #1407
• Part 3: [wallet 3/3]: group key support for channel funding #1413

[coveralls]

Pull Request Test Coverage Report for Build 13571552705

Details

• 392 of 653 (60.03%) changed or added relevant lines in 10 files are covered.
• 30 unchanged lines in 8 files lost coverage.
• Overall coverage increased (+0.05%) to 54.566%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tapsend/send.go	10	11	90.91%
tapfreighter/wallet.go	29	36	80.56%
fn/memory.go	3	13	23.08%
tapgarden/mock.go	18	61	29.51%
rpcserver.go	21	70	30.0%
tapchannel/aux_funding_controller.go	0	65	0.0%
tapfreighter/fund.go	288	374	77.01%

Files with Coverage Reduction	New Missed Lines	%
asset/group_key.go	2	72.65%
asset/mock.go	3	72.74%
itest/multisig.go	3	97.86%
rpcserver.go	3	64.01%
tapgarden/mock.go	3	58.79%
tapchannel/aux_funding_controller.go	5	4.0%
tapchannel/aux_leaf_signer.go	5	43.08%
tapfreighter/chain_porter.go	6	80.57%

Totals
Change from base Build 13569736898:	0.05%
Covered Lines:	48952
Relevant Lines:	89712

---

💛 - Coveralls

[Roasbeef]

Solid set of refactors, no major comments, just a few suggestions and clarifications.

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, 2 of 2 files at r7, 3 of 3 files at r8, 2 of 2 files at r9, 6 of 6 files at r10, 2 of 2 files at r11, 3 of 3 files at r12, 2 of 2 files at r13, 7 of 7 files at r14, 1 of 1 files at r15, 1 of 1 files at r16, 2 of 2 files at r17, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ffranr)

---

tapchannel/aux_funding_controller.go line 2030 at r7 (raw file):

	// AssetSpecifier is the asset that we're funding the channel with.
	AssetSpecifier asset.Specifier

👍

---

tapgarden/interface.go line 400 at r12 (raw file):

	// or when manually rotating something like our current default node
	// key.
	DeriveKey(context.Context,

I think the commit message could use with a bit more detail re why we're removing this method from a seemingly fundamental interface like the KeyRing.

Am actually surprised the removal here didn't break anything? I guess we haven't yet used it for any recovery purposes.

---

tapchannel/aux_funding_controller.go line 2003 at r2 (raw file):

	// them as a single file that can be iterated through.
	proofFile, err := proof.NewFile(proof.V0, fn.Map(
		inputAssetProofs, func(p *proof.Proof) proof.Proof {

Totally non-blocking, but we could condense this further w/ the introduction of an fn.Deref function.

---

tapfreighter/wallet.go line 93 at r9 (raw file):

	// CreatePassiveAssets creates passive asset packets for the given
	// active packets and input Taproot Asset commitments.
	CreatePassiveAssets(ctx context.Context,

Even w/ the function version created, I wonder if it's worth it to retain this method for now?

---

tapchannel/aux_funding_controller.go line 799 at r6 (raw file):

	}
	fundDesc, err := tapsend.DescribeRecipients(
		ctx, pktTemplate, f.cfg.GroupKeyIndex,

Ah, so I guess DescribeRecipients wasn't needed after all. Peeking under the hood, it handled the assetID -> groupKey look up, which is now done via passing around the AssetSpecifier.

---

tapfreighter/fund.go line 294 at r17 (raw file):

	for _, vPkt := range packets {
		for _, vOut := range vPkt.Outputs {
			if err := extractAnchorKey(vOut); err != nil {

Hmm, I thought none of the anchor internal keys would be populated yet at this point?

---

tapfreighter/fund_test.go line 358 at r13 (raw file):

			for i := 0; i < tc.keysDerived; i++ {
				select {
				case <-keyRing.ReqKeys:

I think we can replace this with usage of mock.Mock. We can assert that the function was called a certain amount of times with certain arguments, etc.

[guggero]

Reviewable status: 16 of 17 files reviewed, 3 unresolved discussions (waiting on @ffranr and @Roasbeef)

---

tapchannel/aux_funding_controller.go line 2003 at r2 (raw file):

Previously, Roasbeef (Olaoluwa Osuntokun) wrote…

Totally non-blocking, but we could condense this further w/ the introduction of an fn.Deref function.

I added Deref and DerefPanicas proposed by @ffranr.

---

tapchannel/aux_funding_controller.go line 799 at r6 (raw file):

Previously, Roasbeef (Olaoluwa Osuntokun) wrote…

Ah, so I guess DescribeRecipients wasn't needed after all. Peeking under the hood, it handled the assetID -> groupKey look up, which is now done via passing around the AssetSpecifier.

Yeah, DescribeRecipients is mostly meant for when you don't know what the packet looks like (e.g. it is given to you through the RPC). When you just created the vPacket, it's more clear to follow what happens if you just create the descriptor directly.

---

tapfreighter/fund.go line 294 at r17 (raw file):

Previously, Roasbeef (Olaoluwa Osuntokun) wrote…

Hmm, I thought none of the anchor internal keys would be populated yet at this point?

In most cases it wouldn't be, at least with the way we currently create the template packet.
But the funding code should also work correctly when called with a template that's given to us through the FundVirtualPsbt where we don't actually know what's set and what isn't.

---

tapfreighter/fund_test.go line 358 at r13 (raw file):

Previously, Roasbeef (Olaoluwa Osuntokun) wrote…

I think we can replace this with usage of mock.Mock. We can assert that the function was called a certain amount of times with certain arguments, etc.

Good idea. Done.

---

tapfreighter/wallet.go line 93 at r9 (raw file):

Previously, Roasbeef (Olaoluwa Osuntokun) wrote…

Even w/ the function version created, I wonder if it's worth it to retain this method for now?

Yeah, you're right, makes sense. Also cuts down the number of lines in the diff a bit.

---

tapgarden/interface.go line 400 at r12 (raw file):

Previously, Roasbeef (Olaoluwa Osuntokun) wrote…

I think the commit message could use with a bit more detail re why we're removing this method from a seemingly fundamental interface like the KeyRing.

Am actually surprised the removal here didn't break anything? I guess we haven't yet used it for any recovery purposes.

Updated the commit message. We didn't really use it yet. So can always add it back when we really need it.

[ffranr]

Great improvements

[ffranr]

I don't think this is correct.

rfqmsg.JsonAssetChanInfo seems to have asset level resolution whereas the new capacity value sums over all assets.

With this code change, capacity for each element in resp.Assets will be the sum over all assets. Is that what we want?

[guggero]

Yes, it's weird. But the structure of the JSON has been wrong from the beginning. Each asset piece has the capacity and local/remote balance fields. But those three fields are channel global and not specific to the individual pieces.

So I was planning on refactoring the JSON (hoping that no client applications rely on this structure yet, because it would be a breaking change otherwise) in a future PR. So for now the "capacity" number at least is correctly calculated, even if it's duplicated across multiple asset pieces.
Or do you want me to add the refactor to this PR as well? Given that it's already 17 commits...

[ffranr]

Maybe you should take the tapchannelmsg: fix capacity calculation in JSON commit out of this PR and include it in the JSON refactor PR? The commit may make more sense in that PR.

[guggero]

Okay, I moved the commit to part 3 where I'll also re-arrange the structure of the JSON.

[ffranr]

I'm sure there's a simpler way to do key caching and retrieval here. Not sure...

[guggero]

I refactored it quite a bit, hope this is more clear.