Skip to content
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

close: Print multiple txs; Fixes #6467 #7466

Merged
merged 3 commits into from
Nov 17, 2024

Conversation

ddustin
Copy link
Collaborator

@ddustin ddustin commented Jul 11, 2024

close now outputs txs & txids of all closing transactions (splice candidates can cause there to be multiple).

@ddustin ddustin requested a review from cdecker as a code owner July 11, 2024 19:33
@ddustin ddustin force-pushed the ddustin/close_multi_tx branch from 42b6d65 to 2c3d916 Compare July 11, 2024 20:56
Copy link
Contributor

@niftynei niftynei left a comment

Choose a reason for hiding this comment

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

Can you deprecate the txid field in close?

struct json_stream *result = json_stream_success(cc->cmd);
const struct bitcoin_tx *close_tx = close_txs[tal_count(close_txs) - 1];

json_add_tx(result, "tx", close_tx);
if (!invalid_last_tx(close_tx)) {
struct bitcoin_txid txid;
bitcoin_txid(close_tx, &txid);
json_add_txid(result, "txid", &txid);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: deprecate the txid field, in favor of just using the txids field in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I deprecated txid and tx

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok, missed it the first time! seems like the deprecation feature stuff got updated, very cool.


json_array_start(result, "txids");
for (int i = 0; i < tal_count(close_txs); i++) {
if (invalid_last_tx(close_txs[i])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Man this is an old bugfix (v0.7.0)

@niftynei
Copy link
Contributor

the test failure is from one of the new fetchinvoice/offers tests

cc @rustyrussell
logs_25912008146_fetchinvoice.zip

@ddustin ddustin force-pushed the ddustin/close_multi_tx branch from 2c3d916 to 778aae8 Compare July 18, 2024 18:18
Copy link
Contributor

@niftynei niftynei left a comment

Choose a reason for hiding this comment

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

ACK 778aae8

@rustyrussell
Copy link
Contributor

rustyrussell commented Jul 31, 2024

Rebased, but also deprecated properly, see the second commit.

  1. Deprecated field in the schema now has two values, start and end.
  2. We actually test deprecation status at runtime.
  3. We document all deprecations.
  4. Actually deprecating found a bug in the schema.

You'll note this broke tests, too. This is important; if we break an API we should absolutely feel the pain ourselves!

@rustyrussell rustyrussell force-pushed the ddustin/close_multi_tx branch 2 times, most recently from 94705ed to 9bbada6 Compare July 31, 2024 07:26
@rustyrussell rustyrussell added this to the v24.11 milestone Aug 13, 2024
@rustyrussell rustyrussell force-pushed the ddustin/close_multi_tx branch from 9bbada6 to 3c9942e Compare November 11, 2024 22:51
@rustyrussell
Copy link
Contributor

Trivial rebase.

ddustin and others added 2 commits November 17, 2024 14:28
Changelog-Changed: `close` now outputs txs & txids of all closing transactions (splice candidates can cause there to be multiple).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell force-pushed the ddustin/close_multi_tx branch from 3c9942e to 818ce4b Compare November 17, 2024 03:59
@rustyrussell
Copy link
Contributor

Rebased again, and changed deprecation from 24.08 to 24.11...

Changelog-Deprecated: `close` `tx` and `txid` field (use `txs` and `txids`)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell force-pushed the ddustin/close_multi_tx branch from 818ce4b to 2960251 Compare November 17, 2024 04:01
@rustyrussell rustyrussell merged commit c4cbb86 into ElementsProject:master Nov 17, 2024
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants