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.

Sign up for GitHub

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

Jump to bottom

onchaind now scorches the earth when penalizing a revoked transaction #3870

Merged
merged 7 commits into from
Sep 9, 2020
Merged

onchaind now scorches the earth when penalizing a revoked transaction #3870

merged 7 commits into from
Sep 9, 2020

Conversation

ZmnSCPxj
Copy link
Collaborator

Fixes: #3832

@ZmnSCPxj ZmnSCPxj requested a review from cdecker as a code owner July 23, 2020 10:58
@ZmnSCPxj ZmnSCPxj added this to the Next Release milestone Jul 23, 2020
@cdecker
Copy link
Member

cdecker commented Jul 23, 2020

This looks great, can't wait to dive in and review it, once the release is out the door 🙂

@ZmnSCPxj ZmnSCPxj force-pushed the scorched-earth branch 3 times, most recently from 52e6497 to 17bed2d Compare July 24, 2020 11:54
@ZmnSCPxj
Copy link
Collaborator Author

Rebased, also increased test coverage a little by also checking that an RBFed transaction is considered resolved by the onchaind.

@ZmnSCPxj
Copy link
Collaborator Author

An issue is that this changes the interface of snedrawtransaction. Older plugins which provide sendrawtransaction will not be compatible after this. However, a possible change would be to check if sendrawtransaction with two arguments results in a JSONRPC_INVALID_PARAMS error, and if so, try again with just a single argument, and if this is an RBFed transaction, to print a warning in the log that the plugin is using the old interface and the RBFed transaction may not get forwarded due to too high fee.

@ZmnSCPxj
Copy link
Collaborator Author

Did what I suggested, which is, if deprecated_apis, try to use a single-arg sendrawtransaction if the two-arg sendrawtransaction errors complaining about parameters.

@ZmnSCPxj ZmnSCPxj mentioned this pull request Jul 27, 2020
Open
@ZmnSCPxj
Copy link
Collaborator Author

ZmnSCPxj commented Jul 27, 2020 •
edited
Loading

Rebased. Also, changed the Changelog regarding two-argument sendrawtransaction to "deprecated" (it was "changed"), since we now continue to support the one-argument sendrawtransaction if --deprecated-apis is set.

@ZmnSCPxj ZmnSCPxj mentioned this pull request Jul 30, 2020
Merged
@ZmnSCPxj
Copy link
Collaborator Author

Rebased on top of b934a92 from #3889

@ZmnSCPxj
Copy link
Collaborator Author

ZmnSCPxj commented Aug 4, 2020

Rebased, especially with #3889 merged in.

@ZmnSCPxj
Copy link
Collaborator Author

ZmnSCPxj commented Aug 9, 2020

bump

@ZmnSCPxj ZmnSCPxj mentioned this pull request Aug 18, 2020
Closed
@ZmnSCPxj
Copy link
Collaborator Author

Rebased on master. Also, bump.

@ZmnSCPxj ZmnSCPxj force-pushed the scorched-earth branch 3 times, most recently from b417f27 to ee9d85e Compare August 24, 2020 10:26
@ZmnSCPxj
Copy link
Collaborator Author

ZmnSCPxj commented Aug 24, 2020 •
edited
Loading

Rebased. Also, fixed the capstone --- if we make a penalty transaction that donates the money to miners (because the locktime has been reached) then it previously would make a transaction smaller than the 82-byte limit. We now use a padded OP_RETURN with a pointless 20-byte random commitment to get over the 82-byte limit. Added a test with that extreme case as well.

@cdecker
Copy link
Member

cdecker commented Aug 28, 2020

Assigning @rustyrussell since onchaind is his brainchild :-)

@rustyrussell
Copy link
Contributor

I am going to apply this, though the "real answer" is to have a general engine which makes these kind of "deadline" decisions. That will require careful design, and meanwhile this fixes a real issue.

@rustyrussell
Copy link
Contributor

Ack ee9d85e

@rustyrussell
Copy link
Contributor

(If @ZmnSCPxj doesn't get to it first, I'll rebase and apply...)

@rustyrussell
Copy link
Contributor

Fairly trivial rebase.

@ZmnSCPxj @rustyrussell
onchaind/onchaind_wire.csv: Propagate minimum relay fee to onchaind.
b02f2a1
@ZmnSCPxj @rustyrussell
onchaind/onchain_wire.csv: Allow suppression of auto-rebroadcasting f…
6c26a2c 
…or RBF txes created by onchaind.
@ZmnSCPxj @rustyrussell
bitcoin/tx.c: bitcoin_tx_output_get_amount_sat now accepts const …
b91d42c 
…transactions.

It is a getter, so, does not change the transaction, so should accept `const`.
@ZmnSCPxj @rustyrussell
plugins/bcli.c: sendrawtransaction now has a required `allowhighfee…
1780bd7 
…s` argument.

Changelog-Deprecated: plugin: `bcli` replacements should note that `sendrawtransaction` now has a second required Boolean argument, `allowhighfees`, which if `true`, means ignore any fee limits and just broadcast the transaction. Use `--deprecated-apis` to use older `bcli` replacement plugins that only support a single argument.
@ZmnSCPxj @rustyrussell
bitcoin/script.c: Add scriptpubkey_opreturn_padded, which creates an …
719a89a 
…OP_RETURN with a pointless random 20-byte padding.

In the case of `donateutxo`, this is needed since a simple spend of a P2WPKH to an `OP_RETURN` would be below the minimum transaction size.
Sizes below 20 are not plausible as commitments.
@ZmnSCPxj @rustyrussell
onchaind/onchaind.c: Implement scorch-the-earth.
89719af 
Fixes: ElementsProject#3832

Changelog-Changed: onchaind: We now scorch the earth on theft attempts, RBFing up our penalty transaction as blocks arrive without a penalty transaction getting confirmed.
@ZmnSCPxj @rustyrussell
tests/test_closing.py: Test new onchaind earth-scorching behavior.
8d6442a
@rustyrussell
Copy link
Contributor

... another trivial rebase.

@rustyrussell
Copy link
Contributor

Ack 8d6442a

@rustyrussell rustyrussell merged commit 5bac63c into ElementsProject:master Sep 9, 2020
@ZmnSCPxj ZmnSCPxj deleted the scorched-earth branch October 19, 2021 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdecker cdecker Awaiting requested review from cdecker

@rustyrussell rustyrussell Awaiting requested review from rustyrussell

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.9.1
Development

Successfully merging this pull request may close these issues.

onchaind should scorch-earth our penatly txes
3 participants
@ZmnSCPxj @cdecker @rustyrussell