Skip to content

Test remote fee spike buffer violation #647

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

valentinewallace
Copy link
Contributor

Adds a test for this case that was missing from #577.

Because channel.send_htlc would prevent us from sending a payment that violates our fee spike buffer, each message from the HTLC's sender is manually constructed. Hence why the test looks pretty complicated.

@valentinewallace valentinewallace force-pushed the test-remote-fee-spike-buffer-violation branch from 25abd8c to d277eb3 Compare June 22, 2020 19:41
HTLCs that violate the remote's fee spike buffer should be failed.
@valentinewallace valentinewallace force-pushed the test-remote-fee-spike-buffer-violation branch from d277eb3 to 940d7ac Compare June 22, 2020 19:53
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

LGTM. One coment but its more for future reference since you explicitly check the log entry which is pretty robust.

};
nodes[1].logger.assert_log("lightning::ln::channel".to_string(), "Attempting to fail HTLC due to fee spike buffer violation".to_string(), 1);

check_added_monitors!(nodes[1], 2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not that its really critical, but in general I prefer to check for added monitors right after they get added (ie after the handle_commitment_signed and handle_revoke_and_ack) since it enforces better that those calls succeeded all the way through to generating a monitor update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense 👍

@TheBlueMatt TheBlueMatt merged commit 0133739 into lightningdevkit:master Jun 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants