Fidelity bond wallets #544
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
A brief tour of the wallet. The script generating a wallet now asks the user if it should support fidelity bonds The wallet-tool script has a new method After funding those addresses they appear in the usual wallet-tool display. Locktimes are fixed to midnight on the first of each month, for a maximum locktime until the year 2050. This means that the synchronization code keeps track of 360 addresses per public key. Only mixdepth zero has timelocked addresses and burner output pubkeys. The wallet-tool method Using the wallet-tool method |
chris-belcher
force-pushed
the
fidelity-bonds
branch
2 times, most recently
from
080e4eb to
c83b907
Compare
AdamISZ
reviewed
Mar 25, 2020
AdamISZ
reviewed
Mar 25, 2020
AdamISZ
reviewed
Mar 25, 2020
AdamISZ
reviewed
Mar 25, 2020
AdamISZ
reviewed
Mar 25, 2020
AdamISZ
reviewed
Mar 25, 2020
AdamISZ
reviewed
Mar 25, 2020
AdamISZ
reviewed
Mar 25, 2020
AdamISZ
reviewed
Mar 28, 2020
|
I will incrementally add further notable testing results in this comment to avoid clutter : Some simple regtest tests: Created a timelock address OK. Then went to fund that address from a wallet created via OK I presumptively fixed this by making the references on linke 1002 in Then sent funds to the timelocked address and I tried |
|
I'll leave the above tests until that error in wallet.py is fixed, but just had another thought about this: Recovery: by being BIP49 compatible we gain the ability of users to recover from seedphrase in other wallets - albeit it can be significantly more of a hassle than most wallets, because of both the multiple accounts and (also) potentially the heavy usage with gaps. If we go with more than two address types - that is to say, deviate from the bip32 scheme of 0/1 for external/internal, then you lose the automatically recoverable feature for the timelocked funds. Hmm now I've typed this out I can see the logic though. There is no sense in which some vanilla wallet software will be able to import a custom script like that - unless we are moving into a world where such a thing is standardised. Does miniscript/wallet descriptors come in here somehow? So anyway, I'm happy for it to be like this, as long as there isn't an argument that says "this could be long term incompatible with standard X which is coming" (or does such a standard for timelocks already exist and I missed it? I know of a BIP for an atomic swap script standard but don't think it's used; BIP199 iirc). Did you consider just having a separate account; not because other wallets can currently import it, but because that way it doesn't use a /2 or /3 branch that wouldn't be recognized by other wallets, if such imports became possible in future? (just thoughts; I really don't know). |
AdamISZ
reviewed
Mar 30, 2020
|
Regarding recovery in other wallets, I intend to write one or two BIP documents documenting the format here, so maybe other wallets will support recovering freeze addresses or burner outputs. The only other way to create freeze addresses I'm aware of is this javascript site: https://coinb.in/#newTimeLocked (Although it doesn't create segwit freeze addresses like this PR) When I write the BIPs there might be feedback and then the BIP32 path or serialization can be changed if needed. I briefly considered other accounts (mixdepth = 5) but thought that sometimes tumbler uses more mixdepths than the default in normal usage, so the number of mixdepths cant be limited. Another issue with using another account and using the branches 0/1 is that it might be possible for those same pubkeys to be accidentally reused as regular single-sig addresses. |
chris-belcher
force-pushed
the
fidelity-bonds
branch
2 times, most recently
from
3c6a841 to
a0e57a9
Compare
|
Some notes on the commits I just pushed. Writing them now because they can be useful when later writing documentation and tests. Commits where the first letter of the subject line is lowercase are intended to be squished later. This bunch of commits adds code for supporting burner outputs. Burned output can be created with Embedded in the
The merkle branch of the transaction may be required to prove its existence to third parties. If the block containing the transaction has been pruned then there will be a warning that the merkle branch is unavailable. Note the Any other unpruned Core node can trustlessly obtain the proof and give it to our user with the RPC call First we obtain the proof. Then add it to the wallet. Now sync'ing the wallet doesnt have the no merkle proof warning |
chris-belcher
force-pushed
the
fidelity-bonds
branch
from
12a23cb to
2a8e5dd
Compare
|
Just pushed a commit adding support for watch-only fidelity bond wallets. A brief tour: When fidelity bond wallets are displayed they now highlight a bip32 xpub key. This is done by adding the prefix string This master public key can be used to create a watch-only wallet using a new method of wallet-tool: Once the .jmdat file is created it can be displayed like a normal wallet. Of course only the zeroth mixdepth is displayed and stored because that's where the fidelity bonds are. |
chris-belcher
force-pushed
the
fidelity-bonds
branch
2 times, most recently
from
e336b4a to
d1df086
Compare
|
Pushed commit which adds tests. With this the PR is finished. Removing the WIP tag. |
kristapsk
reviewed
May 2, 2020
AdamISZ
reviewed
May 9, 2020
AdamISZ
reviewed
May 9, 2020
|
I'm continuing to read through all the (non-test at least) code, but one question cropped up pretty much immediately and you could probably save me some thinking time by helping me with the reasoning: Is it not feasible to, either in the wallet class or (possibly, I think not) in the engine class, to make One motivation for that is: I was considering using the new PSBT functionality in #536 to support offline hardware-wallet signing of generic coinjoins or other partially signed with the JM wallet, which should be entirely possible since using BIP49/84, but it makes little to no sense unless we have a watch-only mode for the existing |
|
Right now the Wallet class and cryptoengine class never deals with public keys, only private keys, scripts and addresses. So in this PR I implemented watch-only wallet with a bit of a hack where the watch only wallet private key is actually a public key. So for creating watch-only wallets a mixin wouldn't be enough, I think it would require a slight rewrite where each wallet stores a bip32 pubkey as well as a bip32 privkey. Note that the fidelity bond watchonly wallet only has keys for the zeroth mixdepth, it cant access the other mixdepths because they use hardened key derivation. A further problem with watch only wallets for |
This can be fixed with a very simple patch. diff --git a/scripts/joinmarket-qt.py b/scripts/joinmarket-qt.py
index 2b80352..383f897 100644
--- a/scripts/joinmarket-qt.py
+++ b/scripts/joinmarket-qt.py
@@ -1660,9 +1660,7 @@ class JMMainWindow(QMainWindow):
# only used for GUI display on regtest:
self.testwalletname = wallet.seed = str(firstarg)
if isinstance(wallet, FidelityBondMixin):
- JMQtMessageBox(self, "Fidelity bond wallets not supported by Qt",
- mbtype="warn", title="Error")
- return False
+ raise Exception("Fidelity bond wallets not supported by Qt.")
if 'listunspent_args' not in jm_single().config.options('POLICY'):
jm_single().config.set('POLICY', 'listunspent_args', '[0]')
assert wallet, "No wallet loaded" |
kristapsk
reviewed
May 27, 2020
|
Fixed those review comments by @kristapsk |
|
I've done some basic testing, both with previously created wallets and fidelity bonds wallet, haven't find any new issues so far. Didn't test burning, only timelocking. |
Previously an example of a BIP32 path would be: m/wallet-type'/mixdepth'/internal/index The 'internal' name referred to internal and external addresses (also called change and receive). The renaming to 'address_type' is in preparation to add more branches for timelocked addresses and burner outputs. The variable formally known as 'internal' is now no longer a boolean but always an integer. This almost-always seemlessly fits because the values False and Ture correspond to 0 and 1. The function _get_internal_type therefore has no purpose anymore. Delete it.
The new functions implement creating fund freezing redeem scripts and transactions which spend from such scripts. Also there is a new test function.
Fidelity bond wallets are intended to be used when at a later date using fidelity bonds to greatly increase joinmarket's resistance to sybil attacks. This commit adds support for timelocked addresses. It allows users to optionally create wallet which support such addresses. The synchronization code is modified to also scan for timelocked addresses. The keypairs of the timelocked addresses go in the newly created 2nd address type, where before the zeroth index were receive addresses and first index was change. The locktime dates are fixed at the first of each month for the next 30 years. This means users dont need to remember any dates, and so just their seed phrase and wallet type will still be enough to recover all funds. Each keypair used for timelocking requires an additional 360 addresses to be scanned for, which isn't a problem for Bitcoin Core. Fidelity bonds are only stored in the zeroth mixdepth, as they are not used in repeated coinjoins so theres no point having them in multiple mixdepths. Timelocked addresses don't use the get_new_script() family of functions because they all assume that one index equals one address, and that therefore it's possible to ask for a "next" address. For timelocked addresses knowing the index is not enough to know the address, the timestamp must be known too. Also once one address made of (index, timestamp) is used you mustn't use that index and pubkey again, even though all the other timelocks for that index/pubkey are unused. This is for privacy reasons, as its equivalent to address reuse.
Users burn coins by passing "BURN" as an address to sendpayment
Watch only wallets can now be created via wallet-tool. The wallets store a bip32 xpub key from which all the public keys are generated. Watch only wallets only store and display the zeroth mixdepth, which is the only one needed for fidelity bonds. The bip32 xpub key needed to create a watch only wallet is now specially highlighted in the wallet-tool display, this is to help users actually find it amongst all the other xpubs. The field key_ident in the wallet class was previously generated using private keys, which are not available in watch only wallets. So now for fidelity bond wallets key_ident is generated using a public key. Existing non-fidelity-bond wallets are unaffected
The cryptoengine class BTC_Timelocked_P2WSH now implements sign_transaction() which can be used to spend timelocked UTXOs. FidelityBondMixin.is_timelocked_path() is now used outside the class so its leading underscore has been removed.
Watchonly wallets use pubkeys instead of privkeys, but in a bit of hack the functions previously called "_get_priv_from_path" would actually return public keys for watchonly wallets. This could have pretty terrible consequences one day, so functions like that have been renamed to use the word "key" instead, which could be either private or public.
Previously timelocked UTXOs would be returned by calls like select_utxo() and get_utxos_by_mixdepth(). This caused annoyances if trying to burn a single UTXO. It could also cause recently- unlocked coins to accidently get spent, perhaps co-spent with other coins. This commit fixes that by freezing UTXOs with the coin control feature whenever the wallet is sync'd. When the timelock of a coin passes the user must explicitly use coin control to spend it.
Leave disabled until the rest of the fidelity bond feature is created
chris-belcher
force-pushed
the
fidelity-bonds
branch
from
043d8a9 to
6b41b8b
Compare
|
Good job guys! |
AdamISZ
added a commit
that referenced
this pull request
Jun 6, 2020
Note in particular that: bitcoin.mktx in this PR now does support script entries in outputs to account for nonstandard destinations (as is needed for burn). bitcoin.sign now supports p2wsh (as is needed for timelocks).
AdamISZ
added a commit
that referenced
this pull request
Jul 4, 2020
Note in particular that: bitcoin.mktx in this PR now does support script entries in outputs to account for nonstandard destinations (as is needed for burn). bitcoin.sign now supports p2wsh (as is needed for timelocks).
AdamISZ
added a commit
that referenced
this pull request
Jul 6, 2020
Note in particular that: bitcoin.mktx in this PR now does support script entries in outputs to account for nonstandard destinations (as is needed for burn). bitcoin.sign now supports p2wsh (as is needed for timelocks).
AdamISZ
added a commit
that referenced
this pull request
Jul 10, 2020
41540ab Modify Payjoin code for BIP78 changes. (Adam Gibson) 3ed4e88 Search for correct library extension on mac os (Jules Comte) 6e6bf0a Use VERIFY_STRICTENC flag for Script verification (Adam Gibson) 55295e8 first waypoint on bip78 (Adam Gibson) d34c53b Various fixups: (Adam Gibson) 53ef79b Updates to account for code changes in #544 (Adam Gibson) 4cf77ed Various bugfixes: (Adam Gibson) ca0de5c Add bip78 payjoin module and client-server test: See: https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki (Adam Gibson) ad459d2 Add human readable representations of txs and PSBTs (Adam Gibson) 03a1359 Adds libsecp256k1 installation and addresses reviews (Adam Gibson) 037a2c1 Adds full payjoin workflow test (Adam Gibson) de3ad53 Support output of PSBT instead of broadcast in direct_send (Adam Gibson) f060781 Add SNICKER support to wallets. (Adam Gibson) 22ed0e0 Adds psbt creation and signing support in JM wallet. (Adam Gibson) 070c5bf python-bitcointx backend for jmbitcoin. (Adam Gibson)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A pull request to add to joinmarket wallets that can support fidelity bonds, including watch-only wallets. See this writeup about fidelity bonds and why they are useful: https://gist.github.com/chris-belcher/18ea0e6acdb885a2bfbdee43dcd6b5af/ and also #371
Later pull requests will have the makers and takers actually make use of fidelity bonds. This one is just the wallet side.
Commits in this PR are logically separated to aid in review.
The PR has a few smaller commits which lay the groundwork, including one commit 145eb34 that adds support for freeze addresses which are addresses which use OP_CHECKLOCKTIMEVERIFY to freeze funds until a certain date. This commit will probably conflict slightly with #536, but I skimmed that PR and it doesn't seem too bad.