Prepare for non-Base58 addresses

sipa commented at 1:11 am on August 23, 2017: member

This patch removes the need for the intermediary Base58 type CBitcoinAddress, by providing {Encode,Decode,IsValid}Destination functions that directly operate on the conversion between std::strings and CTxDestination.

As a side, it also fixes a number of indentation issues, and removes probably several unnecessary implicit CTxDestination<->CBitcoinAddress conversions.

This change is far from complete. In follow-ups I’d like to:

Split off the specific address and key encoding logic from base58.h, and move it to a address.h or so.
Replace CTxDestination with a non-boost::variant version (which can be more efficient as boost::variant allocates everything on the heap, and remove the need for boost::get<...> and IsValidDestination calls everywhere).
Do the same for CBitcoinSecret, CBitcoinExtKey, and CBitcoinExtPubKey.

However, I’ve tried to keep this patch to be minimally invasive, but still enough to support non-Base58 addresses. Perhaps a smaller patch is possible to hack Bech32 support into CBitcoinAddress, but I would consider that a move in the wrong direction.

in src/bitcoin-tx.cpp:275 in 36eb9f2c1a outdated

275-    if (!addr.IsValid())
276-        throw std::runtime_error("invalid TX output address");
277-    // build standard output script via GetScriptForDestination()
278-    CScript scriptPubKey = GetScriptForDestination(addr.Get());
279+    CTxDestination destination = DecodeDestination(strAddr);
280+    if (!IsValidDestination(destination)) throw std::runtime_error("invalid TX output address");

promag commented at 2:47 am on August 23, 2017:

Block instead?

0if (...) {
1  throw ...;
2}

in src/rpc/rawtransaction.cpp:401 in 36eb9f2c1a outdated

402+            }
403 
404-            if (setAddress.count(address))
405-                throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, duplicated address: ")+name_);
406-            setAddress.insert(address);
407+            if (destinations.count(destination)) {

promag commented at 2:59 am on August 23, 2017:

Change to

0if (!destinations.insert(destination).second) {

in src/wallet/rpcwallet.cpp:292 in 36eb9f2c1a outdated

291+    if (IsMine(*pwallet, dest)) {
292         // Detect when changing the account of an address that is the 'unused current key' of another account:
293-        if (pwallet->mapAddressBook.count(address.Get())) {
294-            std::string strOldAccount = pwallet->mapAddressBook[address.Get()].name;
295-            if (address == GetAccountAddress(pwallet, strOldAccount)) {
296+        if (pwallet->mapAddressBook.count(dest)) {

promag commented at 3:04 am on August 23, 2017:

Nit, kind of unrelated, use .find() to avoid 2nd lookup below.

sipa commented at 4:08 am on August 23, 2017:

Trying to keep this as much of a straightforward change as possible, as it may need backporting.

in src/wallet/rpcwallet.cpp:459 in 36eb9f2c1a outdated

453@@ -451,9 +454,10 @@ UniValue sendtoaddress(const JSONRPCRequest& request)
454 
455     LOCK2(cs_main, pwallet->cs_wallet);
456 
457-    CBitcoinAddress address(request.params[0].get_str());
458-    if (!address.IsValid())
459+    CTxDestination dest = DecodeDestination(request.params[0].get_str());
460+    if (!IsValidDestination(dest)) {
461         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Bitcoin address");

promag commented at 3:07 am on August 23, 2017:

Follow up, should the error message be Invalid destination address?

sipa commented at 4:07 am on August 23, 2017:

An address is just the string-encoded form of a CTxDestination, so no. I think the “destination” name should be internal.

promag commented at 4:22 am on August 23, 2017:

Off topic, just Invalid address as it seems irrelevant to have Bitcoin in the error message?

sipa commented at 4:23 am on August 23, 2017:

Oh, sure!

in src/wallet/rpcwallet.cpp:2717 in 36eb9f2c1a outdated

2719-           setAddress.insert(address);
2720+            CTxDestination dest = DecodeDestination(input.get_str());
2721+            if (!IsValidDestination(dest)) {
2722+                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string("Invalid Bitcoin address: ") + input.get_str());
2723+            }
2724+            if (destinations.count(dest)) {

promag commented at 3:09 am on August 23, 2017:

Replace with .insert().second like above.

promag commented at 3:14 am on August 23, 2017: member

There is this pattern

0DecodeDestination();
1if (!IsValidDestination()) {

What about:

0CTxDestination dest;
1if (!DecodeDestination(str, dest)) {

Also, to avoid more usages of CBitcoinAddress, add commit to move it to base58.cpp?

sipa commented at 6:38 am on August 23, 2017: member

What about:

0CTxDestination dest;
1if (!DecodeDestination(str, dest)) {

There are plenty of cases where the result of decoding can just be passed to something else, and Function(DecodeDestination(addr)) is far cleaner than CTxDestination dest; DecodeDestination(addr, dest); Function(dest);, so I prefer to keep that possible.

The current pattern is indeed a bit verbose, but once we rewrite CTxDestination to be a native type (rather than a boost::variant; see follow-up plans in the PR description), it could at least simplify to dest.IsValid() or so.

sipa force-pushed on Aug 23, 2017

promag commented at 10:10 am on August 23, 2017: member

@sipa sure, I just wanted to question that “style” because it’s used elsewhere.

ACK 8363a2c.

laanwj added the label Wallet on Aug 23, 2017

jonasschnelli commented at 10:45 am on August 23, 2017: contributor

utACK 8363a2ccaffba793068db4f97184121730707104

laanwj added this to the milestone 0.15.1 on Aug 23, 2017

laanwj commented at 8:11 pm on August 23, 2017: member

Added 0.15.1 milestone

laanwj added the label Needs backport on Aug 23, 2017

kallewoof approved

kallewoof commented at 8:48 am on August 24, 2017: member

utACK 8363a2ccaffba793068db4f97184121730707104 LGTM. I skipped over QT stuff as I don’t know it very well.

in src/qt/addresstablemodel.cpp:85 in 4b1249999b outdated

81@@ -82,14 +82,14 @@ class AddressTablePriv
82             LOCK(wallet->cs_wallet);
83             for (const std::pair<CTxDestination, CAddressBookData>& item : wallet->mapAddressBook)
84             {
85-                const CBitcoinAddress& address = item.first;

theuni commented at 2:22 pm on August 24, 2017:

I’ve stared at this for 5 min now. What trickery made this work before?

sipa commented at 6:08 pm on August 24, 2017:

It was creating a temporary of type CBitcoinAddress by converting the CTxDestination initializer, using the implicit CBitcoinAddress(const CTxDestination &dest) constructor. That temporary’s lifetime was then extended by taking a reference to it, named address.

theuni commented at 6:22 pm on August 24, 2017:

Interesting, I didn’t know an implicit conversion could be done when extending the lifetime of a const ref. Scary!

in src/qt/signverifymessagedialog.cpp:127 in 4b1249999b outdated

128     }
129-    CKeyID keyID;
130-    if (!addr.GetKeyID(keyID))
131-    {
132+    const CKeyID* keyID = boost::get<CKeyID>(&destination);
133+    if (keyID) {

theuni commented at 2:42 pm on August 24, 2017:

!keyID

promag commented at 4:11 pm on August 24, 2017:

Good catch.

sipa commented at 6:12 pm on August 24, 2017:

Wow, thanks :S

in src/script/standard.cpp:322 in 4b1249999b outdated

316@@ -317,3 +317,7 @@ CScript GetScriptForWitness(const CScript& redeemscript)
317     ret << OP_0 << ToByteVector(hash);
318     return ret;
319 }
320+
321+bool IsValidDestination(const CTxDestination& dest) {
322+    return dest.which() != 0;

theuni commented at 3:03 pm on August 24, 2017:

Maybe add a quick comment that this is CNoDestination so it comes up when grepping for future changes.

promag commented at 0:16 am on September 6, 2017:

Could use !(operator==) instead?

in src/script/standard.h:84 in 4b1249999b outdated

76@@ -77,10 +77,13 @@ class CNoDestination {
77  *  * CNoDestination: no destination set
78  *  * CKeyID: TX_PUBKEYHASH destination
79  *  * CScriptID: TX_SCRIPTHASH destination
80- *  A CTxDestination is the internal data type encoded in a CBitcoinAddress
81+ *  A CTxDestination is the internal data type encoded in a bitcoin address
82  */
83 typedef boost::variant<CNoDestination, CKeyID, CScriptID> CTxDestination;
84 
85+/** Check whether a CTxDestination is a CNoDestination. */

theuni commented at 3:04 pm on August 24, 2017:

got it. ignore last nit.

in src/wallet/rpcwallet.cpp:544 in 4b1249999b outdated

542             UniValue addressInfo(UniValue::VARR);
543-            addressInfo.push_back(CBitcoinAddress(address).ToString());
544+            addressInfo.push_back(EncodeDestination(address));
545             addressInfo.push_back(ValueFromAmount(balances[address]));
546             {
547-                if (pwallet->mapAddressBook.find(CBitcoinAddress(address).Get()) != pwallet->mapAddressBook.end()) {

theuni commented at 3:39 pm on August 24, 2017:

heh.

sipa commented at 6:11 pm on August 24, 2017:

It took me a while to figure out what this CBitcoinAddress was doing. Answer: nothing at all.

theuni commented at 3:53 pm on August 24, 2017: member

utACK other than the keyID issue.

sipa force-pushed on Aug 24, 2017

sipa force-pushed on Aug 27, 2017

sipa commented at 5:26 pm on August 27, 2017: member

Removed CBitcoinAddress entirely, using a commit from #11167.

in src/base58.cpp:15 in 4bc71a6cc6 outdated

11@@ -12,6 +12,7 @@
12 #include <string.h>
13 #include <vector>
14 #include <string>
15+#include <algorithm>

promag commented at 9:09 pm on September 1, 2017:

Nit, sort.

sipa commented at 7:51 pm on September 3, 2017:

Fixed.

in src/qt/sendcoinsdialog.cpp:784 in 4bc71a6cc6 outdated

783         if (text.isEmpty()) // Nothing entered
784         {
785             ui->labelCoinControlChangeLabel->setText("");
786         }
787-        else if (!addr.IsValid()) // Invalid address
788+        else if (!IsValidDestination(destination)) // Invalid address

promag commented at 9:50 pm on September 1, 2017:

Nit, fix braces.

sipa commented at 7:52 pm on September 3, 2017:

Not going to reflow this entire function in a PR intended for backporting.

in src/rpc/misc.cpp:218 in 4bc71a6cc6 outdated

217     ret.push_back(Pair("isvalid", isValid));
218     if (isValid)
219     {
220-        CTxDestination dest = address.Get();
221-        std::string currentAddress = address.ToString();
222+        std::string currentAddress = EncodeDestination(dest);

promag commented at 9:53 pm on September 1, 2017:

Could use currentAddress = request.params[0].get_str();?

sipa commented at 7:53 pm on September 3, 2017:

There may be reasons not to do that. Bech32 addresses (added in a later PR) can be uppercase or lowercase, but we always want to store and look up in lowercase form. This is an easy way of doing that, and we should probably check that it’s done consistently elsewhere too.

promag commented at 11:36 pm on September 5, 2017:

Agree, I’m +1 for clear data validation and/or sanitization. Although encode(decode(data)) is a obscure way of doing it, not to mention encode(decode(data)) should equal data?

promag commented at 10:05 pm on September 1, 2017: member

Since this is the base for Bech32, maybe move everything related to CTxDestination out of base58, including tests, to a new place. Also move CTxDestination definition and related functions from src/script/standard.h?

At least should be done in a follow up?

sipa force-pushed on Sep 3, 2017

sipa commented at 8:23 pm on September 3, 2017: member

Since this is the base for Bech32, maybe move everything related to CTxDestination out of base58, including tests, to a new place.

Yes, read the PR description. I plan to do this, but not in a patch that’s intended for backporting.

Also move CTxDestination definition and related functions from src/script/standard.h?

Why? To where?

sipa force-pushed on Sep 5, 2017

instagibbs commented at 9:55 pm on September 5, 2017: member

utACK https://github.com/bitcoin/bitcoin/pull/11117/commits/87d5c28583374f1be6273c3620b29b0a505e0191

though I do not know the QT stuff well.

laanwj added this to the "Blockers" column in a project

promag commented at 11:41 pm on September 5, 2017: member

Removed CBitcoinAddress entirely, using a commit from #11167. @sipa missing this commit as CBitcoinAddress is still around.

sipa commented at 11:44 pm on September 5, 2017: member

@promag Yes, I removed it again.

This PR is just the part that is likely to break easily and need non-trivial rebasing.

in src/qt/transactiondesc.cpp:95 in 87d5c28583 outdated

90@@ -91,9 +91,8 @@ QString TransactionDesc::toHTML(CWallet *wallet, CWalletTx &wtx, TransactionReco
91         if (nNet > 0)
92         {
93             // Credit
94-            if (CBitcoinAddress(rec->address).IsValid())
95-            {
96-                CTxDestination address = CBitcoinAddress(rec->address).Get();
97+            if (IsValidDestination(rec->address)) {
98+                CTxDestination address = DecodeDestination(rec->address);

promag commented at 0:13 am on September 6, 2017:

This incur in a double decoding since above IsValidDestination also decodes. Maybe swap:

0CTxDestination address = DecodeDestination(rec->address);
1if (IsValidDestination(address)) {

This is how it’s done multiples times in this PR.

laanwj commented at 0:20 am on September 6, 2017: member

Not sure if we support this version, but I get the following compile error with clang 3.5.0:

0/home/user/src/bitcoin/src/qt/signverifymessagedialog.cpp:235:40: error: invalid operands to binary expression ('CTxDestination' (aka 'variant<CNoDestination, CKeyID, CScriptID>') and 'CTxDestination')
1    if (CTxDestination(pubkey.GetID()) != destination) {

Then a lot of note: candidate function not viable like:

0/usr/include/x86_64-linux-gnu/qt5/QtCore/qchar.h:544:13: note: candidate function not viable: no known conversion from 'CTxDestination' (aka 'variant<CNoDestination, CKeyID, CScriptID>') to 'QChar' for 1st argument
1inline bool operator!=(QChar c1, QChar c2) { return c1.unicode() != c2.unicode(); }

Edit: Ok, to answer my own question @morcos, the 0.13.0 release notes mention:

0Effectively this means GCC 4.7 or higher, or Clang 3.3 or higher.

So yes we should try to support 3.5.

JeremyRubin commented at 0:23 am on September 6, 2017: contributor

@laanwj

changing it to

if (!(CTxDestination(pubkey.GetID()) == destination)) {

probably fixes it. Ran into this while writing https://github.com/bitcoin/bitcoin/pull/9991

laanwj commented at 0:30 am on September 6, 2017: member

I confirm that @JeremyRubin’s workaround makes it compile.

Edit: after discussion it seems likely that this is due to a boost version difference, not a compiler difference.

sipa force-pushed on Sep 6, 2017

sipa commented at 4:47 pm on September 6, 2017: member

@JeremyRubin Fixed

morcos commented at 7:53 pm on September 6, 2017: member

utACK eebedca

Introduce wrappers around CBitcoinAddress

This patch removes the need for the intermediary Base58 type
CBitcoinAddress, by providing {Encode,Decode,IsValid}Destination
function that directly operate on the conversion between strings
and CTxDestination.

5c8ff0d448

Move CBitcoinAddress to base58.cpp 864cd27874

sipa force-pushed on Sep 6, 2017

laanwj assigned laanwj on Sep 6, 2017

sipa commented at 8:26 pm on September 6, 2017: member

@laanwj Changed to IsValidDestinationString.

laanwj commented at 8:30 pm on September 6, 2017: member

utACK 864cd2787

laanwj merged this on Sep 6, 2017

laanwj closed this on Sep 6, 2017

laanwj referenced this in commit 961901f77e on Sep 6, 2017

laanwj referenced this in commit 089b742a21 on Sep 6, 2017

laanwj removed this from the "Blockers" column in a project

cculianu referenced this in commit 831a13bf63 on Sep 23, 2017

laanwj referenced this in commit aa624b61c9 on Sep 29, 2017

in src/wallet/rpcwallet.cpp:462 in 864cd27874

456@@ -454,9 +457,10 @@ UniValue sendtoaddress(const JSONRPCRequest& request)
457     ObserveSafeMode();
458     LOCK2(cs_main, pwallet->cs_wallet);
459 
460-    CBitcoinAddress address(request.params[0].get_str());
461-    if (!address.IsValid())
462-        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Bitcoin address");
463+    CTxDestination dest = DecodeDestination(request.params[0].get_str());
464+    if (!IsValidDestination(dest)) {
465+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address");

MarcoFalke commented at 8:05 am on October 7, 2017:

nit: Any reason why this was changed from “Invalid Bitcoin address” to “Invalid address” and the other places left as is. This is part of a error string, so shouldn’t break anything I guess.

promag commented at 8:41 am on October 7, 2017:

@achow101 regarding the need for error testing.

MarcoFalke commented at 8:05 am on October 7, 2017: member

(really late) post-merge utACK 864cd278741ab0e17e7d93eedee39417d4797b37

MarcoFalke removed the label Needs backport on Nov 9, 2017

MarcoFalke removed this from the milestone 0.15.2 on Nov 9, 2017

MarcoFalke commented at 7:44 pm on November 9, 2017: member

Removing from backport

ptschip referenced this in commit de7a1b020e on Dec 27, 2017

ptschip referenced this in commit 63dcf54567 on Jan 4, 2018

gandrewstone referenced this in commit 5e864878c6 on Jan 4, 2018

ptschip referenced this in commit ffc945db01 on Jan 5, 2018

gandrewstone referenced this in commit 19aa0660b0 on Jan 5, 2018

zkbot referenced this in commit 2c1f65d6e2 on Apr 23, 2018

zkbot referenced this in commit 1c2f24793b on Apr 24, 2018

zkbot referenced this in commit d5b558ba5b on Apr 25, 2018

zkbot referenced this in commit 962ba99b36 on Apr 25, 2018

zkbot referenced this in commit 88849fa479 on May 3, 2018

zkbot referenced this in commit d97bfb766b on May 4, 2018

niahmiah referenced this in commit 403bf998ce on May 14, 2018

niahmiah referenced this in commit 58d700f304 on May 14, 2018

niahmiah referenced this in commit 00ec878fdf on May 14, 2018

mkjekk referenced this in commit 78344bb4e1 on Aug 12, 2018

mkjekk referenced this in commit cadb67b07b on Aug 12, 2018

PastaPastaPasta referenced this in commit 0ae9068c3e on Sep 24, 2019

PastaPastaPasta referenced this in commit 5a7c790da8 on Sep 24, 2019

FornaxA referenced this in commit a0dafa6b40 on Dec 3, 2019

FornaxA referenced this in commit f0388b35f0 on Dec 3, 2019

FornaxA referenced this in commit 62d3bd4e17 on Dec 4, 2019

FornaxA referenced this in commit 9fab14f78e on Dec 4, 2019

FornaxA referenced this in commit 8e72bd1944 on Dec 4, 2019

FornaxA referenced this in commit 12ee4bae5e on Dec 4, 2019

PastaPastaPasta referenced this in commit 86b9823aa8 on Jan 17, 2020

UdjinM6 referenced this in commit 141861ebae on Jan 22, 2020

PastaPastaPasta referenced this in commit 1e7d9e78d7 on Feb 12, 2020

PastaPastaPasta referenced this in commit bf7167776d on Feb 13, 2020

PastaPastaPasta referenced this in commit a321e6195f on Feb 29, 2020

akshaynexus referenced this in commit 0a2d8c28f0 on May 6, 2020

random-zebra referenced this in commit f59c8fb625 on May 20, 2020

random-zebra referenced this in commit c157deb76b on Jul 5, 2020

cryptolinux referenced this in commit c53adfadce on Feb 6, 2021

cryptolinux referenced this in commit a45c5d4adc on Feb 6, 2021

ckti referenced this in commit a12c875432 on Mar 28, 2021

ckti referenced this in commit c226d4d276 on Mar 28, 2021

ckti referenced this in commit dfdbeba8e8 on Mar 29, 2021

ckti referenced this in commit 7c598f5657 on Mar 29, 2021

gades referenced this in commit 0882df3d0c on Jun 26, 2021

DrahtBot locked this on Sep 8, 2021

Prepare for non-Base58 addresses #11117