-
Notifications
You must be signed in to change notification settings - Fork 961
Giant tx fix #8639
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
Giant tx fix #8639
Conversation
We call it once at the end, but calling on each allocation is excessive, and it shows when dealing with large PSBTS. Testing a 700-input PSBT was unusably slow without this: after this the entire test ran in 9 seconds. Changelog-Fixed: JSON-RPC: Dealing with giant PSBTs (700 inputs!) is now much faster. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
6c0eb3e to
1c84919
Compare
tests/test_plugin.py
Outdated
| """Test that a giant tx doesn't crash bcli""" | ||
| l1 = node_factory.get_node(start=False) | ||
| # With memleak we spend far too much time gathering backtraces. | ||
| del l1.daemon.env["LIGHTNINGD_DEV_MEMLEAK"] |
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 think this line is causing your test to fail. It seems like the LIGHTNINGD_DEV_MEMLEAK key does not exist under valgrind? You can use .pop() instead of del, that way the key will be removed only if it exists.
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 have no idea why the liquid test fails like this though:
FAILED tests/test_plugin.py::test_bitcoin_backend_gianttx - pyln.client.lightning.RpcError: RPC call failed: method: withdraw, payload: {'destination': 'el1qqwawntx90mlylxle6q3vwjcm9r5fc7te6nf58gq469ejxym6fuv5sl6fhxnuat264hf86vmh6h0qupv99flgsy8hhudn3t32f', 'satoshi': 'all'}, error: {'code': -1, 'message': 'Could not parse destination address, destination should be a valid address'}
It seems like l1 is a liquid node, and we're trying to withdraw to a correct liquid address?
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 think this line is causing your test to fail. It seems like the LIGHTNINGD_DEV_MEMLEAK key does not exist under valgrind? You can use .pop() instead of del, that way the key will be removed only if it exists.
pop still raises on missing (according to the docs), so I just tested before delete.
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 have no idea why the liquid test fails like this though:
FAILED tests/test_plugin.py::test_bitcoin_backend_gianttx - pyln.client.lightning.RpcError: RPC call failed: method: withdraw, payload: {'destination': 'el1qqwawntx90mlylxle6q3vwjcm9r5fc7te6nf58gq469ejxym6fuv5sl6fhxnuat264hf86vmh6h0qupv99flgsy8hhudn3t32f', 'satoshi': 'all'}, error: {'code': -1, 'message': 'Could not parse destination address, destination should be a valid address'}It seems like l1 is a liquid node, and we're trying to withdraw to a correct liquid address?
Ah, we have a getnewaddress() wrapper which gives us an unconfidential address: need to use that for elements!
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 think this line is causing your test to fail. It seems like the LIGHTNINGD_DEV_MEMLEAK key does not exist under valgrind? You can use .pop() instead of del, that way the key will be removed only if it exists.
pop still raises on missing (according to the docs), so I just tested before delete.
Sorry, I probably should have elaborated on what I meant when I said to use .pop(), you can use .pop() with a default argument which is returned when the key isn't found in the dictionary. So you can call it like this:
l1.daemon.env.pop("LIGHTNINGD_DEV_MEMLEAK", None)
But a test works too!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
``` lightningd-1 2025-10-27T11:26:04.285Z **BROKEN** plugin-bcli: bitcoin-cli exec failed: Argument list too long ``` Use -stdin to bitcoin-cli: we can then handle arguments of arbitrary length. Fixes: ElementsProject#8634 Changelog-Fixed: plugins: `bcli` would fail with "Argument list too long" when sending a giant tx.
1c84919 to
7ee7407
Compare
Fixes: #8634