-
Notifications
You must be signed in to change notification settings - Fork 63
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
Properly patches sendrawtransaction so it does not crash #220
Conversation
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.
Were the tests crashing without this?
Or just l2
node? Does it recover from crashing?
watchtower-plugin/tests/test.py
Outdated
@@ -48,7 +48,7 @@ def test_watchtower(node_factory, bitcoind, teosd): | |||
locator = change_endianness(dispute_txid[32:]) | |||
|
|||
# Make sure l2's normal penalty_tx doesn't reach the network | |||
l2.daemon.rpcproxy.mock_rpc("sendrawtransaction", lambda: None) | |||
l2.daemon.rpcproxy.mock_rpc("sendrawtransaction", lambda x: {"result": None ,"error": None,"id":"pytest"}) |
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.
nit: replace x
with _
.
The rest were not crashing, but they were logging a bitcoin RPC client crash, given the function called ( CLN can recover from this and no side effect was seen, but I think it's best not to force the crash. |
fa3a2fb
to
fa3c595
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.
I should stop opening github notifications while in bed xD.
Why?
They get marked as read and removed from the notification list.
watchtower-plugin/tests/test.py
Outdated
@@ -48,7 +48,7 @@ def test_watchtower(node_factory, bitcoind, teosd): | |||
locator = change_endianness(dispute_txid[32:]) | |||
|
|||
# Make sure l2's normal penalty_tx doesn't reach the network | |||
l2.daemon.rpcproxy.mock_rpc("sendrawtransaction", lambda: None) | |||
l2.daemon.rpcproxy.mock_rpc("sendrawtransaction", lambda _: {"result": None ,"error": None,"id":"pytest"}) |
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.
Can we properly format this dict:
{"result": None, "error": None, "id": "pytest"}
And, if this RPC call is done multiple times, is it fine for all the answers to have the same "id" ("pytest")?
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.
Properly format as in this is missing a space?
And yeah, the id is just fine. In bitcoin-cli
commands for curl
they always use "curl"
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.
Properly format as in this is missing a space?
Aha + comma position
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.
Sure, I guess it'll be worth adding a black -l 120
check to the python files in the lint CI.
Currently, when mocking sendrawtransaction so it does nothing, the client will raise an exception given the picked lambda does not accept any params (and sendrawtransaction has some). Patches it so this does not happen. This patch is pretty minimal, given the behavior wrt CLN does not change, but it feels better not to except here.
fa3c595
to
7093882
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.
LGTM
Currently, when mocking sendrawtransaction so it does nothing, the client will raise an exception given the picked lambda does not accept any params (and sendrawtransaction has some). Patches it so this does not happen.
This patch is pretty minimal, given the behavior wrt CLN does not change, but it feels better not to except here.