fix: Modified tests to use runner_features #99
Conversation
There was a problem hiding this comment.
Can you organize the commit history by dividing the changes in their own commit?
I do not know if the runner_feature inside the init message will change the meaning of the test, so I need to think more about it
But, I think that the change of the minimum_depth is fine, and it can live in its own commit or PR
I think earlier TryAll was used with multiple init messages as a trial and error method to support multiple implementations, but now we have runner_features which can satisfy all implementations with a single init messages. In some places other bits were send because they were needed in the test so I added them in additional_fields |
| TryAll( | ||
| # BOLT-a12da24dd0102c170365124782b46d9710950ac1 #9: | ||
| # | 20/21 | `option_anchor_outputs` | Anchor outputs | ||
| Msg("init", globalfeatures="", features=bitfield(13, 21)), | ||
| # BOLT #9: | ||
| # | 12/13 | `option_static_remotekey` | Static key for remote output | ||
| Msg("init", globalfeatures="", features=bitfield(13)), | ||
| Msg( | ||
| "init", | ||
| globalfeatures=runner.runner_features(globals=True), | ||
| features=runner.runner_features(additional_features=[12]), |
There was a problem hiding this comment.
This doesn't seem right, and you are changing the meaning of the test here. The previous value was 13, so you should keep 13 because it will tell the node that the feature is optional.
Question for you here: What happens if the runner_features adds feature 12? and the test defined the feature 13?
You now have the feature required and optional, and it is a mess.
There is no definition in the BOLT 9 of this needs to happens here
There was a problem hiding this comment.
I must have mis typed it here. I will fix this.
| Msg( | ||
| "init", | ||
| globalfeatures=runner.runner_features(globals=True), | ||
| features=runner.runner_features(additional_features=[13]), |
There was a problem hiding this comment.
same consideration here
There was a problem hiding this comment.
Here I'm not so sure. I did add 13 as an additional field and 13 was the bit that the test used previously. Does this change the meaning of the test as well?
| Msg( | ||
| "init", | ||
| globalfeatures=runner.runner_features(globals=True), | ||
| features=runner.runner_features(additional_features=[13]), |
There was a problem hiding this comment.
This is a typo as well. I will go through the entire pr to see if there are any more typos and fix them.
|
Please see lightning/bolts#1095 |
|
What is the status of this PR? Can you rebase and see if the CI will pass now? there is a conflict to be resolved |
Tests are updated to use
runner_features, this allows the tests to support multiple lightning implementations at the same time.