-
Notifications
You must be signed in to change notification settings - Fork 100
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
itest: add new testCustomChannelsHtlcSuccess itest #901
Conversation
d53e5ea
to
8754bcb
Compare
b5d747e
to
a03fbdc
Compare
Pushed up a rebase. |
itests are racy right now due to sweeper/block timing, experimenting with either using |
f4e5fcd
to
ec050fb
Compare
ec050fb
to
2c3edb6
Compare
Pushed up a rebase to reference the latest version of lightninglabs/taproot-assets#1154. |
2c3edb6
to
65c7d8f
Compare
Updated to ref the latest |
8754bcb
to
c4900cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Wasn't able to get the test to run on my machine even after many tries. So perhaps this isn't the latest version yet?
itest/litd_custom_channels_test.go
Outdated
// Bob's balance should now reflect that he's gained the value of the HTLC, | ||
// in addition to his settled balance. | ||
// | ||
// TODO(roasbeef): -1 as no decimal display on asset? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that seems to be the likely cause.
itest/litd_custom_channels_test.go
Outdated
// transaction. | ||
mineBlocks(t, net, 1, 1) | ||
|
||
// TODO(roasbeef): assert transfers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asserting that the correct proofs can be found in the universe would also be a great check.
itest/litd_test_list_on_test.go
Outdated
@@ -48,6 +48,10 @@ var allTestCases = []*testCase{ | |||
name: "test custom channels liquidity", | |||
test: testCustomChannelsLiquidityEdgeCases, | |||
}, | |||
{ | |||
name: "test custom channels htlc success", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we'll also want to add a test case for timing out the HTLCs, instead of resolving them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad test name here. We timeout out HTLCs at the very end of the itest when we mine enough blocks to timeout the HTLCs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this tests all 4 scenarios we about ( (local, remote) x (success, timeout) ), it does it all in one sweep with Alice's commitment that she force closed. I'll change the name of the test to better reflect what's covered.
048c654
to
a8e4310
Compare
Pushed up a new version! Ran this for 30 minutes on my laptop and didn't get a flake. If it flakes on CI, I should be able to address it easily, given all the quality time spent with this test. I also added a final assertion to make sure both Alice and Bob can send all the swept funds to a 3rd party. |
c4900cf
to
7d03a39
Compare
a374019
to
d7775e0
Compare
7d03a39
to
7cb58a5
Compare
7cb58a5
to
d82cb9d
Compare
d7775e0
to
72320b2
Compare
Pushed up a rebase. |
itest/litd_custom_channels_test.go
Outdated
proof.UniverseRpcCourierType, zane.Cfg.LitAddr(), | ||
)) | ||
|
||
// Next, we'll make Alice and Bob, who will be the main nodes under test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this and other lines violate the lll
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Old diff? Looks fine to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe, I think there was a fix-up between comment & clicking the "finish review" button
// entire balances to Zane, our 3rd party. | ||
// | ||
// We'll make two addrs for Zane, one for Alice, and one for bob. | ||
zaneTap := newTapClient(t.t, zane) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we keep this we can remove the commented out zane tap init on top of the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean. Zane is the universe server that information is posted to, then we use it at the very end to ensure that we can send the funds we've swept.
This will report balance that we can 100% spend.
This is useful as sometimes we just want to return once the HTLC has been accepted, like when we're paying hodl invoices.
In this commit, we add a new itest that tests the 4 sweeping cases for HTLCs: * local success * remote success * local timeout * remote timeout To test this, we have Alice load up her commitment transaction with 4 HTLCs (2 incoming, 2 outgoing) using hodl invoices. Then we force close her commitment transaction. In this scenario, she needs to broadcast a second level transaction to ultimately timeout, while Bob can sweep directly from the commitment transaction.
657142a
to
f79f326
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gonna merge then absorb the commits into the update-to-lnd-18-4
branch, fixing any nits along the way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
post-merge ack
No description provided.