[go: nahoru, domu]

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

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).

Copy link
Collaborator
@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
Collaborator

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
Collaborator

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
Collaborator

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
Collaborator

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

cc @rustyrussell
logs_25912008146_fetchinvoice.zip

Copy link
Collaborator
@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

Changelog-Changed: `close` now outputs txs & txids of all closing transactions (splice candidates can cause there to be multiple).
@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!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Deprecated: `close` `tx` and `txid` field (use `txs` and `txids`)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell added this to the v24.11 milestone Aug 13, 2024
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.

None yet

3 participants