Skip to content

Delay DelayedPaymentOutput spendable events until the CSV delay #936

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

Conversation

TheBlueMatt
Copy link
Collaborator

No description provided.

@codecov
Copy link

codecov bot commented May 31, 2021

Codecov Report

Merging #936 (f4e4603) into main (58a4f6c) will increase coverage by 0.34%.
The diff coverage is 93.10%.

❗ Current head f4e4603 differs from pull request most recent head e60ccbb. Consider uploading reports for the commit e60ccbb to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #936      +/-   ##
==========================================
+ Coverage   90.53%   90.88%   +0.34%     
==========================================
  Files          60       60              
  Lines       30697    31957    +1260     
==========================================
+ Hits        27792    29044    +1252     
- Misses       2905     2913       +8     
Impacted Files Coverage Δ
lightning/src/util/events.rs 22.41% <ø> (+2.05%) ⬆️
lightning/src/ln/functional_tests.rs 97.16% <88.88%> (+<0.01%) ⬆️
lightning-background-processor/src/lib.rs 94.87% <100.00%> (+0.13%) ⬆️
lightning/src/chain/channelmonitor.rs 91.06% <100.00%> (+0.01%) ⬆️
lightning/src/chain/transaction.rs 96.42% <0.00%> (-3.58%) ⬇️
lightning/src/ln/peer_channel_encryptor.rs 94.20% <0.00%> (-0.94%) ⬇️
lightning/src/util/byte_utils.rs 100.00% <0.00%> (ø)
lightning/src/util/ser_macros.rs 97.59% <0.00%> (+0.04%) ⬆️
lightning/src/ln/msgs.rs 87.89% <0.00%> (+0.09%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58a4f6c...e60ccbb. Read the comment docs.

Comment on lines 306 to 317
fn connect_blocks(node: &mut Node, block_count: u32) {
for i in 1..=block_count {
let prev_blockhash = node.best_block.block_hash();
let height = node.best_block.height() + 1;
let header = BlockHeader { version: 0x20000000, prev_blockhash, merkle_root: Default::default(), time: height, bits: 42, nonce: 42 };
node.best_block = BestBlock::new(header.block_hash(), height);
if i == block_count {
node.node.best_block_updated(&header, height);
node.chain_monitor.best_block_updated(&header, height);
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth de-duplicating confirm_transaction and connect_blocks?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I stared at it a bit but it looked like de-duplicating would have as much boilerplate as code it would avoid. Not sure if you had something specific in mind.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose you can instead just use confirm_transaction but replace ANTI_REORG_DELAY with BREAKDOWN_TIMEOUT as u32 - 1.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! Duh, yes, that would work.

@@ -5507,12 +5509,14 @@ fn test_dynamic_spendable_outputs_local_htlc_success_tx() {
};

mine_transaction(&nodes[1], &node_tx);
connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
connect_blocks(&nodes[1], BREAKDOWN_TIMEOUT as u32 - 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably outside the scope of the PR, but I feel like BREAKDOWN_TIMEOUT could use a better name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, the fact that its public with such an awkward name kinda sucks.

@TheBlueMatt TheBlueMatt force-pushed the 2021-05-spendable-event-locktime-delay branch from 4164176 to 2d0756d Compare June 1, 2021 17:50
@TheBlueMatt TheBlueMatt added this to the 0.0.15 milestone Jun 1, 2021
@TheBlueMatt TheBlueMatt force-pushed the 2021-05-spendable-event-locktime-delay branch from f4e4603 to e60ccbb Compare June 1, 2021 22:33
@TheBlueMatt
Copy link
Collaborator Author

Squashed without diff:

$ git diff-tree -U1 f4e46038 e60ccbb1
$

@TheBlueMatt TheBlueMatt merged commit 1d4f9c8 into lightningdevkit:main Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants