funding: wait for coinbase maturity before sending channel_ready #7925
Conversation
Crypt-iQ
added
the
funding
Crypt-iQ
force-pushed
the
funding_maturity
branch
from
bc208e5
to
84ee8db
Compare
Crypt-iQ
force-pushed
the
funding_maturity
branch
from
84ee8db
to
d0b5929
Compare
Crypt-iQ
force-pushed
the
funding_maturity
branch
3 times, most recently
from
782863c
to
d525d90
Compare
| // Send along the oneConfChannel again and then assert that the open | ||
| // event is sent. This serves as the 100 block + MinAcceptDepth | ||
| // confirmation. | ||
| alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ | ||
| Tx: fundingTx, | ||
| } |
There was a problem hiding this comment.
This mock test does exercise the new code, but it's brittle. It does not actually test that coinbase maturity has been reached or that the channel can be closed immediately after channel_ready.
I think an itest is needed to properly test what we want.
There was a problem hiding this comment.
To test this in an itest, this needs to use the psbt flow and bitcoind as a miner to generate directly to the p2wsh via generatetoaddress. However, we use btcd as itest miner which doesn't have a generatetoaddress analog. We can't stop btcd and change the --miningaddr flag because the itests are parallel and another test may be using the miner. This unit test isn't ideal, but does give some coverage. I have also tested this on my machine several times with psbt's and bitcoind and verified that the channel was unusable until after 100+min depth confs. Unfortunately, I don't have time to change btcd's code to add the generatetoaddress rpc so I think this is the best we can do right now
| alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ | ||
| Tx: fundingTx, | ||
| } | ||
|
|
||
| bob.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ | ||
| Tx: fundingTx, | ||
| } |
There was a problem hiding this comment.
This is revealing to me that theres some structural issues with the mock notifier. It makes some assumptions about the number of confirmations with the different channels and hardcodes the 6 conf case which doesn't make a tremendous amount of sense to me. Although that issue precedes this one, this change makes it clear that this test setup is improperly designed. At minimum it seems like the two channel setup might want to track 1. whether the tx has been mined, and 2. whether it has achieved the minimum depth.
I don't have any specific guidance on how to rectify this but I wanted to raise the issue in case others had some input on how to deal with it.
This is needed so that the next commit can create a ShimIntent without having to export the ShimIntent's fields.
This tests that the funding manager doesn't immediately consider a coinbase transaction that is also a funding transaction usable until the maturity has been reached.
Crypt-iQ
force-pushed
the
funding_maturity
branch
from
d525d90
to
8f20cd8
Compare
|
@Crypt-iQ, remember to re-request review from reviewers when ready |
No description provided.