Full BIP173 (Bech32) support #11167
Full BIP173 (Bech32) support #11167
Conversation
|
Would prefer to have simply sending-to (maybe validating/analyzing too?) as a separate PR, before wallet upgrades. I'm not sure when it would make sense to convert between P2SH and BIP173... |
|
@luke-jr I agree, but I consider
I think you're right. I'll remove that. |
|
addwitnessaddress is very much not actual support, it's a test shim. |
|
But it modifies the wallet, no? Seems useful to review independently from the rest. Especially since it has the additional considerations of what happens if you try to use it and then downgrade to an older version... |
|
@luke-jr Consider that we've since 0.13.1 had support for receiving and spending native witness outputs in the wallet (without that, testing the consensus logic for it would have been much harder), just no way to encode such outputs as strings. So I think the encoding is somewhat orthogonal. It does modify the wallet, but I'm not sure it's worth trying to separate the logic. We only have one data type ( Wallet backward compatibility is only affected when you use an import or |
|
Added support in Python framework, and some integrated some functional tests into the |
src/bech32.cpp
Outdated
There was a problem hiding this comment.
IMO bool Decode(const std::string& str, const std::string& hrp, data& d) feels better, and this way below it can early return.
There was a problem hiding this comment.
It's not often used in the Bitcoin Core codebase, but using pairs for multiple returned values is very typical in C++ (see the return type of std::map::insert for example). In C++03 it was a bit verbose to use, but with C++11's auto types and std::tie for assigning to multiple variables, it's pretty convenient. I'd rather stick with the current approach.
src/base58.cpp
Outdated
src/base58.cpp
Outdated
src/bech32.h
Outdated
src/base58.cpp
Outdated
There was a problem hiding this comment.
Must come first? If not which is the cheapest?
There was a problem hiding this comment.
bech32 is far cheaper (no basis conversion, no SHA256).
src/base58.cpp
Outdated
There was a problem hiding this comment.
Must come first? If not which is the cheapest?
There was a problem hiding this comment.
I haven't benchmarked, but Bech32 should be far cheaper (no SHA256, no basis conversion). There should never be overlap between the addresses, so the order shouldn't matter.
There was a problem hiding this comment.
Right, in terms of functionality the order doesn't matter. But at the moment most addresses (don't know numbers) are base58 so for now move bech32::Decode() after DecodeBase58Check()?
It would be cool to move this out of base58.cpp, follow up maybe?
There was a problem hiding this comment.
Right, in terms of functionality the order doesn't matter. But at the moment most addresses (don't know numbers) are base58 so for now move bech32::Decode() after DecodeBase58Check()?
I was using a fail-fast approach, making the thing that most quickly fails first. You're right that as long as there are hardly any bech32 addresses, putting Base58 would be overall faster. But none of this is performance critical anyway...
src/chainparams.cpp
Outdated
There was a problem hiding this comment.
I arbitrarily chose a bech32 prefix for regtest. Feel free to bikeshed (it doesn't even need to be just 2 characters).
src/bech32.cpp
Outdated
src/bech32.cpp
Outdated
src/bech32.cpp
Outdated
src/bech32.h
Outdated
There was a problem hiding this comment.
Could you leave a comment referring the reader to more complete documentation on bech32 (either the BIP or reference repo)? Also would be nice to document somewhere in the codebase what hrp stands for.
I'm thinking something as simple as: "Bech32 is a data encoding used for some newer address formats. Output consists of a human-readable part (HRP) followed by a separator, then the data itself with a checksum. For more details, see documentation of BIP 173."
There was a problem hiding this comment.
Done again - I somehow lost this change.
There was a problem hiding this comment.
I don't see the comment... I think you lost it again.
There was a problem hiding this comment.
It's there, just a bit higher up in the file.
src/test/base58_tests.cpp
Outdated
There was a problem hiding this comment.
nit: Maybe use toupper here to be more descriptive and succinct.
There was a problem hiding this comment.
Unfortunately, toupper is locale-dependent, so it can't be used for consistent behaviour.
There was a problem hiding this comment.
std::toupper(c, std::locale::classic())?
sipa
force-pushed
the
201708_bech32
branch
3 times, most recently
from
d7dd8a5 to
f77636b
Compare
|
Include a test with v1+ address? |
Refactor t-address encoding Includes code cherry-picked from the following upstream PRs: - bitcoin/bitcoin#11117 - bitcoin/bitcoin#11259 - Only the second commit (first is for QT code) - bitcoin/bitcoin#11167 - Only the first commit (the rest are not part of the t-address encoding refactor). Part of zcash#3058. Precursor to zcash#3202.
Refactor t-address encoding Includes code cherry-picked from the following upstream PRs: - bitcoin/bitcoin#11117 - bitcoin/bitcoin#11259 - Only the second commit (first is for QT code) - bitcoin/bitcoin#11167 - Only the first commit (the rest are not part of the t-address encoding refactor). Part of zcash#3058. Precursor to zcash#3202.
Function extracted from upstream: PR bitcoin#11167 Commit c091b99
Function extracted from upstream: PR bitcoin#11167 Commit c091b99
0e1ac93 CMake: fix pivx-qt linking (Fuzzbawls) 6512f74 CMake: add more verbose checking for required binary programs (Fuzzbawls) 54a5bbb Build: clean rust target directory when calling make clean (Fuzzbawls) c3e99fe Naming standardization: moving listsaplingaddresses to listshieldedaddresses. (furszy) ed6adcd Add backwards compatible implementation of explicit_bzero (Fuzzbawls) 43474d5 Fetch rust packages from our own depends cache (Fuzzbawls) b50f983 Quote unquoted echo in gitian descriptors (Fuzzbawls) 4d7db4c Wallet upgrade to Sapling features implemented. (furszy) 52c435e Allow for custom zk params dir (Fuzzbawls) d0f7dc9 Tests: don't run sapling test on non-HD wallets (Fuzzbawls) 1f1adac move sapling initializer functions to src/util (Fuzzbawls) 4f34cf6 Use the fs wrapper for sapling util operations (Fuzzbawls) aa0adc0 TravisCI: Add support for librustzcash (Fuzzbawls) b667db9 Doc: note new dependencies (Fuzzbawls) 1d3c409 Build: Add support for building librustzcash with autotools (Fuzzbawls) 594c1f4 CMake: add support for building librustzcash (Fuzzbawls) 8445e9e Depends: Move librustzcash crate to source tree (Fuzzbawls) 5eec150 Fix missing wallet lock in StoreAndLoadSaplingZkeys. (furszy) e158cb4 Fetch zkSNARKs params into our own PIVXParams folder. (furszy) 8dc509c Trailing whitespaces cleanup over Sapling files (furszy) adb0a62 Migrated sapling classes to the new serialization methods (furszy) 9d26c92 Missing argument in LogPrintf fix (furszy) e88aac5 Store seed id & path metadata in Sapling keys. (furszy) aedf460 Fix linux compiler issues (furszy) 88db05b Fix duplicate TryCreateDirectory in linux. (furszy) f99de4c Fix g++ missing std::array (furszy) 659e452 [Dependes] rust is not optional anymore. (furszy) 813bfd6 [Dependes] Use Rust 1.42.0 (furszy) b0a2d9a Dev tooling, LIBRUSTZCASH_OVERRIDE. (furszy) 1913715 Update .gitignore for Rust code (Jack Grigg) e0c0978 rust: Adjust Cargo.toml so that it compiles (Jack Grigg) 4f404ab rust: Pin toolchain to 1.36.0, matching depends system (Jack Grigg) 9eccd61 Bring in Cargo.lock from librustzcash repo (Jack Grigg) f520872 Bring in librustzcash crate (Jack Grigg) 397e68b [Test] Remove invalid address comparison from WriteCryptedSaplingZkeyDirectToDb. (furszy) 15f464f test_runner: include sapling prefix (furszy) aabf416 Upgrade librustzcash to 0.2.0 (Jack Grigg) 94f23b6 Fixing duplicate wallet_test_fixture definition in makefile.test (furszy) 463a340 depends: Add missing cargo path (furszy) 06a6f78 Simplify locking, merge cs_SpendingKeyStore into cs_KeyStore (furszy) b40105c depends: Support additional cross-compilation targets in Rust (furszy) 35de114 [Sapling] Pass parameter paths as native strings to librustzcash (furszy) 41d0a9a [Sapling] Persist Sapling payment address to IVK map (furszy) da2724f [Test] Coverage for loadSaplingKey and metadata. (furszy) 3e5f653 [BugFix] Wallet force new keypool in setupGeneration. (furszy) e8d2776 [Wallet] Sapling Crypter moved to Sapling spkm (furszy) 58e3667 [UNDO] wallet_zkeys_test (furszy) d018a0e [Test] Move wallet_zkeys_test to TestingSetup (furszy) 4015db8 [Wallet] Fix empty unlockSaplingsKeys. (furszy) c2bc104 Fix m_spk_man->SetupGeneration missing force param in encrypt wallet (furszy) 7920df2 Wallet: SetupSPKM prevent keypool generation in unit tests. (furszy) c6243af Sapling libraries included in qt binaries. (furszy) 76fcd87 Implement GetSaplingExtendedSpendingKey. (furszy) 1b193ae [Wallet] load Sapling hd chain from disk. (furszy) dbc4306 [Refactor] Sapling keys, address doesn't need to be optional. (furszy) 4fee93f Sapling wallet persistence added to functional tests (furszy) ee8680a [Test] Sapling addresses persistence functional test. (furszy) 8e33a7a [RPC] Implement list sapling addresses. (furszy) 3d4b303 Test suite, include assert_true and assert_false util methods (furszy) c7847dc Persist Sapling key material in the wallet to disk. (furszy) 6cb12cd [Wallet] Store ExtFVK with encrypted Sapling spending key instead of FVK (furszy) ad3f499 [Wallet] Sapling HD chain setup. (furszy) 47eef0b Switch from SaplingSpendingKey to SaplingExtendedSpendingKey. (furszy) 2338e18 [Sapling] Raise the 90-character limit on Bech32 encodings. (furszy) a386b2d [Refactor] Use CPrivKey typedef instead exactly equal RawHDSeed new definition. (furszy) 7bb5c3d Fix test_bitcoin circular dependency issue. (furszy) b487a2c Sapling spkm: Store Sapling key metadata indexed by ivk (furszy) 3dd93e6 Sapling spkm: SetHDSeed method created. (furszy) 41fe77f [Sapling] CHDChain getter and setter. (furszy) 417df65 [HDChain] Chain counter type supporting Standard and Sapling counters. (furszy) aabcda4 [Move-Only] Sapling script pub key manager class created. (furszy) 6576fa7 [Test] Use regtest in sapling/key_tests/ps_address_test (furszy) 4207872 [Test] Sapling addresses encoding and decoding. (furszy) 838eac6 [Sapling] Implement CKeyStore::GetSaplingPaymentAddresses() . (furszy) 2bb4c9a [Sapling] Pass SaplingPaymentAddress to store through the CKeyStore (furszy) 47c83c2 [Sapling] Add CWallet::AddCryptedSaplingSpendingKey() hook. (furszy) 99567ba [Sapling] Check for unencrypted Sapling keys in CCryptoKeyStore::SetCrypted() (furszy) ada3bb7 [Sapling] Add Sapling decryption check to CCryptoKeyStore::Unlock(). (furszy) c52a018 [Sapling] Add Sapling keys to CCryptoKeyStore::EncryptKeys. (furszy) 6494a62 [Sapling] Add Sapling have/get sk crypter overrides. (furszy) 7eeb914 [Test] Sapling wallet RPC validateaddress unit test. (furszy) 3059ca1 [Sapling][RPC] validateaddress: include sapling address information. (furszy) 2a4dc0b [Wallet] HaveSpendingKeyForPaymentAddress method implemented. (furszy) cb56b37 [Sapling][RPC] getnewsaplingaddress method created. (furszy) 06b6966 [Sapling] Load zk-snarks params. (furszy) da60be2 [Sapling] init zk-snarks params. (furszy) dce8d15 [Sapling] back port fetch params (furszy) bec02b5 [Test] Back port WalletTestingSetup. (furszy) db0ab99 [Sapling] Use CChainParams::Bech32HRP() in zs_address_test (furszy) 9dfdf20 [Sapling] Add comment about size calculations for converted serialized keys. (furszy) b5f6461 [Sapling] key_io unit test back ported (furszy) 397ca1f [Sapling] Spending key encode/decode methods back ported. (furszy) a446ec2 [Sapling] key_io basic methods back ported. (furszy) a6ec5d2 Fix bech32::Encode() error handling (Jack Grigg) 9fdfc24 Simplify Base32 and Base64 conversions (furszy) eadb9da Generalize ConvertBits (Pieter Wuille) a7d5e44 ConvertBits() - convert from one power-of-2 number base to another. Function extracted from upstream: PR bitcoin#11167 Commit c091b99 (furszy) f31f47b chainparams: Add Sapling Bech32 HRPs (furszy) 04fe6c4 Import Bech32 C++ reference code & tests (Pieter Wuille) f6cb741 [Sapling] Add crypted keystore sapling add key. (furszy) 859d31e Implement CSecureDataStream. (furszy) dcac242 Abstract CDataStream object. (furszy) 016ab38 [Sapling] Change default_address to return SaplingPaymentAddr and not boost::optional (furszy) d7c12a1 [Sapling] Add StoreAndRetrieveSaplingSpendingKey unit test. (furszy) 54d67dd [Sapling] Add sapling keystore unit test. (furszy) ffd01e9 [Sapling] Add SaplingIncomingViewingKeys map, SaplingFullViewingKey methods. (furszy) d19d4b1 [Sapling] Add Sapling Add/Have/Get to keystore. (furszy) 1f93139 [Sapling] make diversifier functions return option. (furszy) cb4f8a1 [Sapling] zip32 back ported + zip32 unit tests. (furszy) acbf54b uint256: blob88 created (furszy) c1ce452 [Sapling] Back port: sapling note tests. (furszy) 2609281 [Sapling] Back port: sapling utils unit test. (furszy) 99577fd [Sapling] Back port: noteencryption unit test. (furszy) 0a76014 [Sapling] Back port: note, noteEncryption and address classes. (furszy) b1529d0 [Sapling] Back port util file. (furszy) 6dfc4d2 Add blake2b writer. (furszy) d903ccb [Ser] Serialize/unserialize arrays capabilities. (furszy) 6d0b7a7 [Sapling] Sapling constants included. (furszy) 6896ad2 [Sapling] Update librustzcash with ZIP32 APIs (furszy) fe08568 [Sapling] prf integrated. (furszy) 72dfdfa [Backport] sapling lib updated. (furszy) 459560b [Backport] Added SHA256Compress to SHA256 implementation. (furszy) 816a083 [Depends] libsodium added. (furszy) aaa625f [Depends][Test] Update librustzcash and sapling-crypto + remove redundant rust_tests.cpp. (furszy) 5e27e45 [Depends][Test] Update pedersen_hash_test to account for updated librustzcash encoding. (furszy) e8c4d72 [Depends] Configure librustzcash for cross-compiling. (furszy) a3f9a82 [Depends] further cross-compiling work, rust mingw32 included. (furszy) 8303947 [Depends] librustzcash updated. (furszy) a5a122e [Test] Invoke the merkle_hash API in librustzcash via test suite. (furszy) 9d2acd7 [depends] librustzcash update. (furszy) 954db5e [depends] Update to latest librustzcash with sapling-crypto dependencies. (furszy) 1d6e808 [Depends] Add support for unpackaged Rust crates. (furszy) fe03449 depends: Fix regex bugs in cargo-checksum.sh (Jack Grigg) af12248 [Depends] Explicitly download and vendor Rust dependencies. (furszy) e78d9f1 [Build] rust, platform specific files and hashes (linux added) (furszy) 35c5882 [Build] Include rust and rustzcash libraries into our system :) . (furszy) Pull request description: Foundational PR towards full Sapling integration. Covering, in a nutshell, the first two milestones: #### 1) "Build & depend/deterministic system configuration support". Build system configuration basis for Sapling. Rust, libsodium, librustzcash (Sapling rust library - up until version 0.2.0) + all of the needed crates dependencies (~34 libraries). #### 2) "Wallet: Sapling keys & addresses management. Generation/storage/encryption following ZIP32 derivation linked to wallet's BIP44 master seed". New address format support, bech32 for Sapling payment addresses. Sapling keys deterministic derivation from the wallet's master seed (same as regular/staking addresses. wallet's seed will restore PIVs and Shielded PIVs equally). Sapling Keys/Addresses persistence on disk and encryption/decryption process connected to the wallet. Support for extended full viewing key, incoming viewing key, outgoing viewing key and spending key (Further explanation [here](https://github.com/zcash/zips/blob/master/zip-0032.rst)). New/Updated RPC method calls added: `getnewshieldedaddress` —> generating new bech32 Sapling address and adding it to the keystore encrypted/unencrypted. `listsaplingaddresses` —> listing wallet Sapling addresses. `validateaddress` → covering Sapling addresses support. Test Coverage. - Unit tests Sapling unit tests are under `/src/test/librust/` directory. - Functional tests Sapling functional tests are under `/test/functional/` directory following the new prefix `sapling_**.py` . NOTE: Remember to fetch the params using the script in util/fetch-params.sh ----------------------- — Next PR will cover the Sapling wallet transactions' management (commits history will not be as incremental as this foundational PR, needed to do it in this way because of the substantial amount of work in the build system area. Aiming to get us directly up-to-date with the latest work in the area). ACKs for top commit: Fuzzbawls: ACK 0e1ac93 random-zebra: ACK 0e1ac93 and merging... 🎉 Tree-SHA512: 568941f19d077fe5b74297447b1d648743c5560c1fc389d8c61c0c01532c29df0837140875eefb199584df2e71bdd49a72297a9a4bc4ed528cb1e4f20febaf13
Implement {Encode,Decode}Destination without CBitcoinAddress
Import Bech32 C++ reference code & tests
This includes a reformatted version of the Bech32 reference code
(see https://github.com/sipa/bech32/tree/master/ref/c%2B%2B), with
extra documentation.
Convert base58_tests from type/payload to scriptPubKey comparison
Add regtest testing to base58_tests
Implement ConvertBits
A part of "Implement BIP173 addresses and tests"
Make linter happy
92f1f8b Split off key_io_tests from base58_tests (Pieter Wuille) 119b0f8 Split key_io (address/key encodings) off from base58 (Pieter Wuille) ebfe217 Stop using CBase58Data for ext keys (Pieter Wuille) 32e69fa Replace CBitcoinSecret with {Encode,Decode}Secret (Pieter Wuille) Pull request description: This PR contains some of the changes left as TODO in bitcoin#11167 (and built on top of that PR). They are not intended for backporting. This removes the `CBase58`, `CBitcoinSecret`, `CBitcoinExtKey`, and `CBitcoinExtPubKey` classes, in favor of simple `Encode`/`Decode` functions. Furthermore, all Bitcoin-specific logic (addresses, WIF, BIP32) is moved to `key_io.{h,cpp}`, leaving `base58.{h,cpp}` as a pure utility that implements the base58 encoding/decoding logic. Tree-SHA512: a5962c0ed27ad53cbe00f22af432cf11aa530e3efc9798e25c004bc9ed1b5673db5df3956e398ee2c085e3a136ac8da69fe7a7d97a05fb2eb3be0b60d0479655 Make linter happy Dashify
An overview of commits that implement an initial switch from using CBitcoinAddress to CTxDestination using bech32 (in line with Bitcoin's implementation). A 80% implementation (not cleaned up) can be found in our archives: - cevap/ion-old-v3@3b30fd0 - cevap/ion-old-v3@f8ee3a8 - cevap/ion-old-v3@7f87033 - cevap/ion-old-v3@f0d2206 - cevap/ion-old-v3@dbddc7b - cevap/ion-old-v3@e834f27 - cevap/ion-old-v3@2c467e1 - cevap/ion-old-v3@ef36201 - cevap/ion-old-v3@3d6e391 Implementing this switch in full is future work, which needs: - bitcoin#11117 - bitcoin#11167 - bitcoin#11372
An overview of commits that implement an initial switch from using CBitcoinAddress to CTxDestination using bech32 (in line with Bitcoin's implementation). A 80% implementation (not cleaned up) can be found in our archives: - cevap/ion-old-v3@3b30fd0 - cevap/ion-old-v3@f8ee3a8 - cevap/ion-old-v3@7f87033 - cevap/ion-old-v3@f0d2206 - cevap/ion-old-v3@dbddc7b - cevap/ion-old-v3@e834f27 - cevap/ion-old-v3@2c467e1 - cevap/ion-old-v3@ef36201 - cevap/ion-old-v3@3d6e391 Implementing this switch in full is future work, which needs: - bitcoin#11117 - bitcoin#11167 - bitcoin#11372
19 participants
Builds on top of #11117.
This adds support for:
addwitnessaddress, though by default it still produces P2SH versions)validateaddress)It includes a reformatted version of the C++ Bech32 reference code and an independent implementation of the address encoding/decoding logic (integrated with CTxDestination). All BIP173 test vectors are included.
Not included (and intended for other PRs):