Skip to content

tests: include the test for the tests #137

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 7, 2025
Merged

tests: include the test for the tests #137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tests/test_bolt1-02-unknown-messages.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

from typing import Any

from lnprototest import TryAll, Connect, ExpectMsg, Msg, RawMsg, Runner
from lnprototest import Connect, ExpectMsg, Msg, RawMsg, Runner
from lnprototest.event import ExpectDisconnect
from lnprototest.utils import run_runner

Expand Down
12 changes: 7 additions & 5 deletions tests/test_bolt2-01-open_channel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
# Variations on open_channel, accepter + opener perspectives
from lnprototest import (
TryAll,
OneOf,
Sequence,
ExpectDisconnect,
Block,
FundChannel,
ExpectMsg,
ExpectTx,
ExpectError,
Msg,
RawMsg,
AcceptFunding,
Expand All @@ -26,8 +25,6 @@
remote_funding_privkey,
bitfield,
Block,
ExpectError,
Disconnect,
)
from lnprototest.stash import (
sent,
Expand Down Expand Up @@ -291,6 +288,8 @@ def test_open_channel_opener_side(runner: Runner) -> None:
def test_open_channel_opener_side_wrong_announcement_signatures(runner: Runner) -> None:
"""Testing the case where the channel is announces in the correct way but one node
send the wrong signature inside the `announcement_signatures` message."""
from lnprototest.clightning import Runner as CLightningRunner

connections_events = connect_to_node_helper(
runner=runner,
tx_spendable=tx_spendable,
Expand All @@ -302,6 +301,8 @@ def test_open_channel_opener_side_wrong_announcement_signatures(runner: Runner)

dummy_sign = "138c93afb2013c39f959e70a163c3d6d8128cf72f8ae143f87b9d1fd6bb0ad30321116b9c58d69fca9fb33c214f681b664e53d5640abc2fdb972dc62a5571053"
short_channel_id = opts["short_channel_id"]

is_cln = isinstance(runner, CLightningRunner)
test_events = [
# BOLT 2:
#
Expand Down Expand Up @@ -342,6 +343,7 @@ def test_open_channel_opener_side_wrong_announcement_signatures(runner: Runner)
#
# So we should change the OneOf to all exception and stop when
# the first one succided
ExpectDisconnect(),
Sequence(ExpectDisconnect(), enable=is_cln),
Sequence(ExpectMsg("error"), enable=(not is_cln)),
Comment on lines +346 to +347
Copy link
Contributor

Choose a reason for hiding this comment

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

I have some questions.

  1. Why are we using Sequence instead of OneOf? Shouldn't this be a OneOf list of acceptable responses? That way we might not have to specify is_cln?
  2. Instead of ExpectMsg("error") maybe we can use ExpectError().

I was going through the BOLT specification and found

The recipient:
if signature is incorrect OR non-compliant with LOW-S-standard rulehttps://github.com/bitcoin/bitcoin/pull/6769:
MUST send a warning and close the connection, or send an error and fail the channel.

We are checking for Disconnect(closing connection) and Error, maybe we should also check channel closure?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shouldn't this be a OneOf list of acceptable responses?

Yes, but now the OneOf event will read through the buffer and consume the buffer, so the next event will read from an incorrect buffer. The Sequence allows for disabling the execution for each run.

There could be smarter way to use OneOf, but I am working on a new API that allows a better way of writing tests, so I will stay with what we have!

We are checking for Disconnect(closing connection) and Error, maybe we should also check channel closure?

Right, but we do not have anyone who is now sending warnings. When we have them, we should add the warning there.

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 probably the ExpectDisconnect when find the connection ready, and it is able to consume the buffer can stash the info inside the Runner, and then ExpectMsg check for stashed message :/ but this is so complicated.

We can track this with an issue? In this way we can track it?

]
run_runner(runner, merge_events_sequences(pre_events, test_events))
Loading