Use CWallet::Import* functions in all import* RPCs #16301

achow101 commented at 12:53 AM on June 28, 2019: member

#15741 introduced ImportPrivKeys, ImportPubKeys, ImportScripts, and ImportScriptPubKeys in CWallet which are used by importmulti. This PR changes the remaining import* RPCs (importaddress, importprivkey, importpubkey, and importwallet) to use these functions as well instead of directly adding the imported items to the wallet.

achow101 force-pushed on Jun 28, 2019

achow101 commented at 12:55 AM on June 28, 2019: member

This also makes the CWallet restructure simpler as the changes to CWallet's import functions will then apply to all the import* RPCs.

fanquake added the label Wallet on Jun 28, 2019

achow101 force-pushed on Jun 28, 2019

DrahtBot commented at 4:19 AM on June 28, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16341 (WIP: Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)
#16037 (rpc: Early fail import(wallet,multi) when a required block is pruned by promag)
#15414 ([wallet] allow adding pubkeys from imported private keys to keypool by Sjors)
#15093 (rpc: Change importwallet to return additional errors by marcinja)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

MarcoFalke added the label Refactoring on Jun 28, 2019

MarcoFalke commented at 8:59 PM on June 28, 2019: member

Approach ACK

in src/wallet/rpcdump.cpp:725 in e66ed3b845 outdated

 716 |      pwallet->chain().showProgress("", 100, false); // hide progress dialog in GUI
 717 |      RescanWallet(*pwallet, reserver, nTimeBegin, false /* update */);
 718 |      pwallet->MarkDirty();
 719 |  
 720 | -    if (!fGood)
 721 | -        throw JSONRPCError(RPC_WALLET_ERROR, "Error adding some keys/scripts to wallet");

promag commented at 10:42 PM on June 29, 2019:

e66ed3b84593bebd3550e18b5b3be3605e08ced1

These changes are kind of breaking change, the caller has no way to know if something went bad - as it is will always succeed.Is this the case just because dumpwallet/importwallet should not be used?

achow101 commented at 12:05 AM on July 2, 2019:

This change was because it ImportScripts and ImportPrivKeys doesn't return why it failed. It could fail because the script or privkey already exists in the wallet which would still be fine, or it could cail because something went wrong with writing the data to the wallet file which is bad. I suppose I could make them return an enum, but that's a larger change that I didn't really want to do.

instagibbs commented at 2:39 PM on July 10, 2019:

The errors reported as-is appear to at least be somewhat helpful.

It could fail because the script or privkey already exists in the wallet

I don't think that's true. These two checks appear to be only reporting real errors. Perhaps I missed something.

achow101 commented at 12:29 AM on July 12, 2019:

Yes, but these checks are part of ImportPrivKeys and ImportScripts both of which can return false in non-error cases. Both have HaveKey and/or HaveCScript checks that can fail, but not cause the import to be bad. But there could also be an actual error which would result in the import being bad.

ryanofsky commented at 8:22 PM on July 16, 2019:

In commit "Have importwallet use ImportPrivKeys and ImportScripts" (991ecfdbbb289bd6e4c50fc63f9201edbca14f70)

Both have HaveKey and/or HaveCScript checks that can fail, but not cause the import to be bad.

What's this referring to? Import methods seem to return true if imported data was already found. But maybe I'm misinterpreting. Might help to refer to a specific example.

achow101 commented at 11:06 PM on July 16, 2019:

Turns out I was having a hard time with the negations.

I've pushed another commit that makes it more explicit when something is being skipped because we already have it, as well as logging when this happens.

promag commented at 10:52 PM on June 29, 2019: member

Concept ACK. Spent some time reviewing the changes commit by commit and this mostly looks good to me considering latests refactors to the wallet. IMO last commit should be reworked or add a explanation as to why it's safe to ignore errors/return values.

in src/wallet/rpcdump.cpp:186 in ea665e323f outdated

 181 | +
 182 |          {
 183 |              pwallet->MarkDirty();
 184 |  
 185 | +            // Use timestamp of 1 to scan the whole chain
 186 | +            if (!pwallet->ImportPrivKeys(privkey_map, 1)) {

ryanofsky commented at 3:14 PM on July 1, 2019:

In commit "Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys" (ea665e323f76766def6c4f46a46c53e38365d179)

Function would be a simpler without the privkey_map variable. You could write pwallet->ImportPrivKeys({{vchAddress, key}}, 1)

achow101 commented at 12:06 AM on July 2, 2019:

Done

in src/wallet/rpcdump.cpp:197 in ea665e323f outdated

 200 | -            // Don't throw error in case a key is already there
 201 | -            if (pwallet->HaveKey(vchAddress)) {
 202 | -                return NullUniValue;
 203 | +                CScript script = GetScriptForDestination(dest);
 204 | +                script_pub_keys.insert(script);
 205 | +                pwallet->ImportScriptPubKeys(strLabel, {script}, true, false, 1, (!request.params[1].isNull()) || pwallet->mapAddressBook.count(dest) == 0);

ryanofsky commented at 3:16 PM on July 1, 2019:

In commit "Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys" (ea665e323f76766def6c4f46a46c53e38365d179)

It would be easier to understand what this call was doing if it included argument names: ..., /* have_solving_data= */ true, /* internal= */ false, /* timestamp= */ 1, /* apply_label= */ ...

achow101 commented at 12:06 AM on July 2, 2019:

Done everywhere

in src/wallet/wallet.cpp:1720 in 12fd3a849c outdated

1716 | @@ -1717,7 +1717,7 @@ bool CWallet::ImportPubKeys(const std::vector<CKeyID>& ordered_pubkeys, const st
1717 |      return true;
1718 |  }
1719 |  
1720 | -bool CWallet::ImportScriptPubKeys(const std::string& label, const std::set<CScript>& script_pub_keys, const bool have_solving_data, const bool internal, const int64_t timestamp)
1721 | +bool CWallet::ImportScriptPubKeys(const std::string& label, const std::set<CScript>& script_pub_keys, const bool have_solving_data, const bool internal, const int64_t timestamp, bool apply_label)

ryanofsky commented at 3:30 PM on July 1, 2019:

In commit "Optionally apply labels to imported scriptPubKeys" (12fd3a849c4be0879e48e65661ca1628c21308b2)

Having two internal and apply_labels parameters that both control the same behavior is unnecessarily confusing. I think it'd be best to drop internal and only keep apply_labels since it's more explicit, but you could just as easily not make any change here and just keep using internal.

achow101 commented at 12:06 AM on July 2, 2019:

Removed apply_labels and the commit that adds it.

in src/wallet/rpcdump.cpp:202 in ea665e323f outdated

 213 | -                throw JSONRPCError(RPC_WALLET_ERROR, "Error adding key to wallet");
 214 | +            // Add the wpkh script for this key if possible
 215 | +            if (pubkey.IsCompressed()) {
 216 | +                pwallet->ImportScripts({GetScriptForDestination(WitnessV0KeyHash(vchAddress))});
 217 |              }
 218 | -            pwallet->LearnAllRelatedScripts(pubkey);

ryanofsky commented at 3:47 PM on July 1, 2019:

In commit "Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys" (ea665e323f76766def6c4f46a46c53e38365d179)

Just curious, but why is this replacing the LearnAllRelatedScripts() call with inlined code? The change seems fine but is duplicative, makes code longer, and seems like the opposite of the other changes here switching to higher level ImportPrivKeys() and ImportScriptPubKeys() calls.

achow101 commented at 9:55 PM on July 1, 2019:

This was done in preparation for the wallet restructure which causes LearnAllRelatedScripts to no longer be part of CWallet.

Sjors commented at 4:19 PM on July 9, 2019:

I actually find this slightly more readable than the previous LearnAllRelatedScripts -> LearnRelatedScripts(key, OutputType::P2SH_SEGWIT);. .... However it's hard to tell if it's really doing the same thing. Maybe add an extra commit to make that more clear?

ryanofsky commented at 5:20 PM on July 17, 2019:

In commit "Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys" (5776e6d72e76e03251d346588035fcdb0423440c)

Just for the record, I like Sjors idea of putting the AddKeyPubKey->ImportPrivKeys and LearnAllRelatedScripts->ImportScripts in two separate commits, since they are independent changes. But it's not very important.

in src/wallet/rpcdump.cpp:514 in 94ad6ad965 outdated

 510 | @@ -511,7 +511,7 @@ UniValue importpubkey(const JSONRPCRequest& request)
 511 |              }.ToString());
 512 |  
 513 |  
 514 | -    std::string strLabel;
 515 | +    std::string strLabel = "";

ryanofsky commented at 3:50 PM on July 1, 2019:

In commit "Have importpubkey use CWallet's ImportScriptPubKeys and ImportPubKeys functions" (94ad6ad965ba6dfffaf7387b15ac659323fc85d0)

Accidental change? (It's more verbose and less efficient.)

achow101 commented at 12:06 AM on July 2, 2019:

Reverted

in src/wallet/rpcdump.cpp:559 in 94ad6ad965 outdated

 557 | +
 558 | +        std::vector<CKeyID> ordered_pubkeys = {pubKey.GetID()};
 559 | +        std::map<CKeyID, CPubKey> pubkey_map = {{pubKey.GetID(), pubKey}};
 560 | +        std::map<CKeyID, std::pair<CPubKey, KeyOriginInfo>> key_origins;
 561 | +
 562 | +        pwallet->ImportPubKeys(ordered_pubkeys, pubkey_map, key_origins, false, false, 1);

ryanofsky commented at 3:52 PM on July 1, 2019:

In commit "Have importpubkey use CWallet's ImportScriptPubKeys and ImportPubKeys functions" (94ad6ad965ba6dfffaf7387b15ac659323fc85d0)

Would be good to label arguments: /* add_keypool= */ false, /*internal = */ false, /* timestamp= */ 1

in src/wallet/rpcdump.cpp:553 in 94ad6ad965 outdated

 551 | -        ImportScript(pwallet, GetScriptForRawPubKey(pubKey), strLabel, false);
 552 | -        pwallet->LearnAllRelatedScripts(pubKey);
 553 | +
 554 | +        pwallet->MarkDirty();
 555 | +
 556 | +        pwallet->ImportScriptPubKeys(strLabel, script_pub_keys, true, false, 1);

ryanofsky commented at 4:04 PM on July 1, 2019:

In commit "Have importpubkey use CWallet's ImportScriptPubKeys and ImportPubKeys functions" (94ad6ad965ba6dfffaf7387b15ac659323fc85d0)

Would be good to label arguments: /* have_solving_data = */ true, /*internal = */ false, /* timestamp= */ 1

in src/wallet/rpcdump.cpp:548 in 94ad6ad965 outdated

 542 | @@ -543,11 +543,20 @@ UniValue importpubkey(const JSONRPCRequest& request)
 543 |          auto locked_chain = pwallet->chain().lock();
 544 |          LOCK(pwallet->cs_wallet);
 545 |  
 546 | +        std::set<CScript> script_pub_keys;
 547 |          for (const auto& dest : GetAllDestinationsForKey(pubKey)) {
 548 | -            ImportAddress(pwallet, dest, strLabel);
 549 | +            script_pub_keys.insert(GetScriptForDestination(dest));
 550 |          }
 551 | -        ImportScript(pwallet, GetScriptForRawPubKey(pubKey), strLabel, false);

ryanofsky commented at 4:14 PM on July 1, 2019:

In commit "Have importpubkey use CWallet's ImportScriptPubKeys and ImportPubKeys functions" (94ad6ad965ba6dfffaf7387b15ac659323fc85d0)

Just a note to other reviewers: this seems to happen in ImportPubKeys now

instagibbs commented at 2:01 PM on July 10, 2019:

more specifically here's the rough logic of ImportScript:

mark dirty 

addwatchonly

if is redeem script:
  addcscript
  ImportAddress
else
  SetAddressBook

And in this case it's not a redeemscript(last arg is false), so it marks dirty, adds watchonly, and sets address book.

Mark dirty is top-level now, ImportScriptPubKeys takes care of the SetAddressBookWithDB, and both ImportScriptPubKeys and ImportPubKeys adds watch only in specific situations(when pubkey is not already in wallet, or the script is not in the wallet yet). Previously the watchonly script was added when ISMINE was not spendable. I'll leave it up to other reviewers to figure if this is desired/same semantics.

ryanofsky commented at 7:32 PM on July 16, 2019:

I'll leave it up to other reviewers to figure if this is desired/same semantics. @achow101 could you clarify in commit message whether this causes a change in behavior or not? If you aren't sure, it'd at least be good to say if it potentially changes behavior. This is hurting my head to think about but it seems only potential difference is whether timestamp is overridden when key already exists?

For all these commits, it'd be really helpful to note in commit messages whether they do or don't or maybe change behavior and what the scope of the change is.

achow101 commented at 11:07 PM on July 16, 2019:

I've added a note to each commit indicating what behavior changes in it.

Here, the only behavior change is that the timestamp for the key will be set to 1 if we already have it.

in src/wallet/wallet.h:1095 in 92863d7de8 outdated

1091 | @@ -1092,7 +1092,7 @@ class CWallet final : public CCryptoKeyStore, private interfaces::Chain::Notific
1092 |      bool DummySignTx(CMutableTransaction &txNew, const std::vector<CTxOut> &txouts, bool use_max_sig = false) const;
1093 |      bool DummySignInput(CTxIn &tx_in, const CTxOut &txout, bool use_max_sig = false) const;
1094 |  
1095 | -    bool ImportScripts(const std::set<CScript> scripts) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
1096 | +    bool ImportScripts(const std::set<CScript> scripts, int64_t time = -1) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

ryanofsky commented at 8:40 PM on July 1, 2019:

In commit "Optionally allow ImportScripts to set script creation timestamp" (92863d7de8cf0d7432bc47f36bf1aa4de257c548)

I think it'd be better not to have a default parameter so callers will have a prompt to provide this information if it's available and not just drop it in the floor. There shouldn't be a lot of preexisting calls.

achow101 commented at 12:06 AM on July 2, 2019:

Done

in src/wallet/wallet.cpp:1672 in 92863d7de8 outdated

1668 |      for (const auto& entry : scripts) {
1669 |          if (!HaveCScript(CScriptID(entry)) && !AddCScriptWithDB(batch, entry)) {
1670 |              return false;
1671 |          }
1672 | +
1673 | +        if (time >= 0) {

ryanofsky commented at 8:47 PM on July 1, 2019:

In commit "Optionally allow ImportScripts to set script creation timestamp" (92863d7de8cf0d7432bc47f36bf1aa4de257c548)

This is inconsistent with existing code which uses 0 to indicate an unknown time and 1 to indicate the earliest timestamp.

Would suggesting replacing (time >= 0) to (time > 0) here and below and changing default to 0 instead of -1. (Or dropping the default entirely).

achow101 commented at 12:06 AM on July 2, 2019:

Done

ryanofsky commented at 8:53 PM on July 1, 2019: member

Really nice changes. Almost finished reviewing (will update list below with progress).

12fd3a849c4be0879e48e65661ca1628c21308b2 Optionally apply labels to imported scriptPubKeys (1/6)
ea665e323f76766def6c4f46a46c53e38365d179 Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys (2/6)
94ad6ad965ba6dfffaf7387b15ac659323fc85d0 Have importpubkey use CWallet's ImportScriptPubKeys and ImportPubKeys functions (3/6)
2fb78f8baad0778de81dc81209340e04ccef4df6 Have importaddress use ImportScripts and ImportScriptPubKeys (4/6)
92863d7de8cf0d7432bc47f36bf1aa4de257c548 Optionally allow ImportScripts to set script creation timestamp (5/6)
e66ed3b84593bebd3550e18b5b3be3605e08ced1 Have importwallet use ImportPrivKeys and ImportScripts (6/6)

achow101 force-pushed on Jul 2, 2019

in src/wallet/rpcdump.cpp:685 in cf2d2945b9 outdated

 682 | @@ -699,31 +683,20 @@ UniValue importwallet(const JSONRPCRequest& request)
 683 |              const CScript& script = script_pair.first;
 684 |              int64_t time = script_pair.second;
 685 |              CScriptID id(script);

practicalswift commented at 1:38 PM on July 2, 2019:

id seems to be unused?

achow101 commented at 7:04 PM on July 9, 2019:

Removed it.

in src/wallet/rpcdump.cpp:183 in e3cc8e102f outdated

 184 | +                throw JSONRPCError(RPC_WALLET_ERROR, "Error adding key to wallet");
 185 | +            }
 186 | +
 187 |              // We don't know which corresponding address will be used;
 188 |              // label all new addresses, and label existing addresses if a
 189 |              // label was passed.

ryanofsky commented at 5:28 PM on July 2, 2019:

In commit "Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys" (74ec96b601dd6417f01f4801d83e01eeb7dded18)

It's not really clear from context that this comment is referring to the apply_label argument passed to ImportScriptPubKeys below. Would suggest moving the comment or mentioning this explicitly.

achow101 commented at 11:08 PM on July 16, 2019:

Reverted the below change entirely.

in src/wallet/rpcdump.cpp:194 in e3cc8e102f outdated

 192 | -                if (!request.params[1].isNull() || pwallet->mapAddressBook.count(dest) == 0) {
 193 | -                    pwallet->SetAddressBook(dest, strLabel, "receive");
 194 | -                }
 195 | +                CScript script = GetScriptForDestination(dest);
 196 | +                script_pub_keys.insert(script);
 197 | +                pwallet->ImportScriptPubKeys(strLabel, {script}, true /* have_solving_data */, (request.params[1].isNull()) && pwallet->mapAddressBook.count(dest) != 0 /* internal */ , 1 /* timestamp */ );

ryanofsky commented at 5:41 PM on July 2, 2019:

In commit "Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys" (e3cc8e102fc2736b3871869646f8d60599988f82)

Is this ever going to add watch only scripts given the !IsMine check in ImportScriptPubKeys?

It'd be nice to have a succinct comment here saying when this will add a watch only script, and why it's necessary.

instagibbs commented at 1:51 PM on July 10, 2019:

(request.params[1].isNull()) && pwallet->mapAddressBook.count(dest) != 0 /* internal */

This seems like an extreme abuse of the flag based on the name and confused me. This should be renamed add_to_book_if_valid or maybe even better just have the validity check inside SetToAddressBook(do we ever want to import invalid destinations?) and call it add_to_addrbook.

achow101 commented at 1:44 AM on July 12, 2019:

I've renamed the flag to apply_label.

achow101 commented at 10:12 PM on July 16, 2019:

It won't ever add the scripts as watch only because we import the private keys earlier.

achow101 commented at 11:08 PM on July 16, 2019:

Reverted this change

in src/wallet/rpcdump.cpp:186 in e3cc8e102f outdated

 188 |              // label all new addresses, and label existing addresses if a
 189 |              // label was passed.
 190 | +            std::set<CScript> script_pub_keys;
 191 |              for (const auto& dest : GetAllDestinationsForKey(pubkey)) {
 192 | -                if (!request.params[1].isNull() || pwallet->mapAddressBook.count(dest) == 0) {
 193 | -                    pwallet->SetAddressBook(dest, strLabel, "receive");

Sjors commented at 4:23 PM on July 9, 2019:

It's not obvious to me how dropping SetAddressBook doesn't change behavior, e.g. we're no longer calling NotifyAddressBookChanged.

achow101 commented at 5:46 PM on July 9, 2019:

SetAddressBook happens in ImportScriptPubKeys

instagibbs commented at 1:46 PM on July 10, 2019:

As long as the imported key is !internal and is valid destination, it does appear to SetAddressBookWithDB which in this RPC is true.

Sjors commented at 4:24 PM on July 9, 2019: member

Concept ACK. I have some questions about the first commit, will study the rest later.

achow101 force-pushed on Jul 9, 2019

in src/wallet/rpcdump.cpp:182 in e3cc8e102f outdated

 174 | @@ -175,31 +175,29 @@ UniValue importprivkey(const JSONRPCRequest& request)
 175 |          CPubKey pubkey = key.GetPubKey();
 176 |          assert(key.VerifyPubKey(pubkey));
 177 |          CKeyID vchAddress = pubkey.GetID();
 178 | +
 179 |          {
 180 |              pwallet->MarkDirty();
 181 |  
 182 | +            // Use timestamp of 1 to scan the whole chain

instagibbs commented at 1:42 PM on July 10, 2019:

why 1 and not 0? No internal documentation I can find on this.

achow101 commented at 11:51 PM on July 11, 2019:

Dunno, it's just adapted from the changed code.

jonasschnelli commented at 4:02 PM on July 16, 2019:

1 was introduced in #7551...

ryanofsky commented at 5:22 PM on July 16, 2019:

why 1 and not 0? No internal documentation I can find on this.

As far as I know, there is no substantive difference anymore between 0 and 1, but I think it's good to keep using 1 here so this is a refactoring change, not a change in behavior.

I do think in general it's better to use 1 as an explicit request to rescan from the beginning of the chain as opposed to 0 which could indidcate we're dealing with an old wallet that didn't save metadata (see #9108, #9682), or an uninitialized variable, or some other undefined craziness from all the spaghetti code.

in src/wallet/rpcdump.cpp:195 in e3cc8e102f outdated

 206 | -            pwallet->UpdateTimeFirstKey(1);
 207 | -            pwallet->mapKeyMetadata[vchAddress].nCreateTime = 1;
 208 | -
 209 | -            if (!pwallet->AddKeyPubKey(key, pubkey)) {
 210 | -                throw JSONRPCError(RPC_WALLET_ERROR, "Error adding key to wallet");
 211 | +            // Add the wpkh script for this key if possible

instagibbs commented at 1:56 PM on July 10, 2019:

? This is already done by GetAllDestinationsForKey

achow101 commented at 12:02 AM on July 12, 2019:

ImportScriptPubKeys doesn't actually add the scripts to the wallet, so we add the script here.

instagibbs commented at 2:18 PM on July 12, 2019:

Might be useful to say as much

in src/wallet/rpcdump.cpp:552 in 28f30ca209 outdated

 550 | +
 551 | +        pwallet->MarkDirty();
 552 | +
 553 | +        pwallet->ImportScriptPubKeys(strLabel, script_pub_keys, true /* have_solving_data */, false /*internal */, 1 /* timestamp */);
 554 | +
 555 | +        pwallet->ImportPubKeys({pubKey.GetID()}, {{pubKey.GetID(), pubKey}}, {}, false /* add_keypool */, false /* internal */, 1 /* timestamp */);

instagibbs commented at 2:05 PM on July 10, 2019:

can you annotate key_origins too?

achow101 commented at 1:45 AM on July 12, 2019:

Done

in src/wallet/wallet.h:1095 in f9115eb2d6 outdated

1091 | @@ -1092,7 +1092,7 @@ class CWallet final : public CCryptoKeyStore, private interfaces::Chain::Notific
1092 |      bool DummySignTx(CMutableTransaction &txNew, const std::vector<CTxOut> &txouts, bool use_max_sig = false) const;
1093 |      bool DummySignInput(CTxIn &tx_in, const CTxOut &txout, bool use_max_sig = false) const;
1094 |  
1095 | -    bool ImportScripts(const std::set<CScript> scripts) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
1096 | +    bool ImportScripts(const std::set<CScript> scripts, int64_t time) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

instagibbs commented at 2:25 PM on July 10, 2019:

nit: timestamp since you're calling it that in other places and it matches better other args

achow101 commented at 1:45 AM on July 12, 2019:

Done

in src/wallet/rpcdump.cpp:686 in 041e65458e outdated

 698 | -            }
 699 | -            if(!pwallet->AddCScript(script)) {
 700 | -                pwallet->WalletLogPrintf("Error importing script %s\n", HexStr(script));
 701 | -                fGood = false;
 702 | +
 703 | +            if (pwallet->ImportScripts({script}, time)) {

instagibbs commented at 2:28 PM on July 10, 2019:

isn't this backwards?

achow101 commented at 12:09 AM on July 12, 2019:

How so?

instagibbs commented at 2:19 PM on July 12, 2019:

Because it's reporting errors when it returns true?

achow101 commented at 11:08 PM on July 16, 2019:

Oh, whoops. Fixed.

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

Change ImportScriptPubKeys' internal to apply_label

The internal bool was only to indicate whether the given label should
be applied as things that are internal should not have a label. To make
this clearer, we change internal to apply_label and invert its usage
so things that have labels set this to true in order to have their labels
applied.

ab28e31c95

achow101 force-pushed on Jul 12, 2019

Sjors commented at 2:11 PM on July 12, 2019: member

Travis sad <img width="1212" alt="Schermafbeelding 2019-07-12 om 15 11 21" src="https://user-images.githubusercontent.com/10217/61134332-54c9ea00-a4b7-11e9-8213-8202e004a94c.png">

achow101 commented at 8:32 PM on July 12, 2019: member

Fixed travis hopefully.

achow101 force-pushed on Jul 12, 2019

instagibbs commented at 1:58 AM on July 16, 2019: member

utACK 991ecfdbbb289bd6e4c50fc63f9201edbca14f70

in src/wallet/rpcdump.cpp:194 in 74ec96b601 outdated

 192 | -                if (!request.params[1].isNull() || pwallet->mapAddressBook.count(dest) == 0) {
 193 | -                    pwallet->SetAddressBook(dest, strLabel, "receive");
 194 | -                }
 195 | +                CScript script = GetScriptForDestination(dest);
 196 | +                script_pub_keys.insert(script);
 197 | +                pwallet->ImportScriptPubKeys(strLabel, {script}, true /* have_solving_data */, (!request.params[1].isNull()) || pwallet->mapAddressBook.count(dest) == 0 /* apply_label */ , 1 /* timestamp */ );

ryanofsky commented at 6:45 PM on July 16, 2019:

In commit "Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys" (74ec96b601dd6417f01f4801d83e01eeb7dded18)

I don't understand why this is calling ImportScriptPubKeys instead of just SetAddressBook like before. This is confusing because the code is first going out of its way to remove associated watch-only scripts from setWatchOnly (in AddKeyPubKeyWithDB via ImportPrivKeys):

https://github.com/bitcoin/bitcoin/blob/74ec96b601dd6417f01f4801d83e01eeb7dded18/src/wallet/wallet.cpp#L310-L319

And then after removing the scripts it looks like it goes out of its way to add them by calling ImportScriptPubKeys. But then the ImportScriptPubKeys call doesn't add the scripts because have_solving_data and IsMine are both true in the following check?

https://github.com/bitcoin/bitcoin/blob/74ec96b601dd6417f01f4801d83e01eeb7dded18/src/wallet/wallet.cpp#L1724-L1727

Something seems wrong here. If scripts are meant to be added to setWatchOnly it seems like the removal code shouldn't be called and the adding code might need to be fixed. Of the scripts are meant to be removed, then it's misleading to call ImportScriptPubKeys instead of just SetAddressBook like before.

achow101 commented at 11:08 PM on July 16, 2019:

Reverted this change

in src/wallet/rpcdump.cpp:191 in 74ec96b601 outdated

 196 | +                script_pub_keys.insert(script);
 197 | +                pwallet->ImportScriptPubKeys(strLabel, {script}, true /* have_solving_data */, (!request.params[1].isNull()) || pwallet->mapAddressBook.count(dest) == 0 /* apply_label */ , 1 /* timestamp */ );
 198 |              }
 199 |  
 200 | -            // Don't throw error in case a key is already there
 201 | -            if (pwallet->HaveKey(vchAddress)) {

ryanofsky commented at 7:11 PM on July 16, 2019:

In commit "Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys" (74ec96b601dd6417f01f4801d83e01eeb7dded18)

It looks like there is a minor change in behavior here due to implementation of ImportPrivKeys:

https://github.com/bitcoin/bitcoin/blob/74ec96b601dd6417f01f4801d83e01eeb7dded18/src/wallet/wallet.cpp#L1683

Previously nCreateTime would not be updated if key already existed, and now it will be (in memory only, apparently the value is not written to disk on the next line).

Would suggesting fixing up ImportPrivKeys to avoid this change in behavior, or documenting in the commit message here that there is a slight change in behavior.

achow101 commented at 11:09 PM on July 16, 2019:

Fixed ImportPrivKeys

in src/wallet/rpcdump.cpp:353 in d2b7db13b9 outdated

 319 | +            pwallet->MarkDirty();
 320 | +
 321 | +            pwallet->ImportScriptPubKeys(strLabel, {script}, false /* have_solving_data */, true /* apply_label */, 1 /* timestamp */);
 322 |          } else if (IsHex(request.params[0].get_str())) {
 323 |              std::vector<unsigned char> data(ParseHex(request.params[0].get_str()));
 324 | -            ImportScript(pwallet, CScript(data.begin(), data.end()), strLabel, fP2SH);

ryanofsky commented at 7:39 PM on July 16, 2019:

In commit "Have importaddress use ImportScripts and ImportScriptPubKeys" (d2b7db13b92391c87adf6db862593b311c7cbdee)

Slight change of behavior here, if fP2SH is false and the provided scriptpubkey is spendable, this code used throw "wallet already contains the private key" error and now it doesn't. This seems like a good change.

achow101 commented at 11:10 PM on July 16, 2019:

To be consistent, I've also removed the fail if HaveKey above. This results in labels being updated if things already existed.

in src/wallet/rpcdump.cpp:326 in d2b7db13b9 outdated

 323 |              std::vector<unsigned char> data(ParseHex(request.params[0].get_str()));
 324 | -            ImportScript(pwallet, CScript(data.begin(), data.end()), strLabel, fP2SH);
 325 | +            CScript redeem_script(data.begin(), data.end());
 326 | +
 327 | +            std::set<CScript> scripts = {redeem_script};
 328 | +            pwallet->ImportScripts(scripts);

ryanofsky commented at 7:51 PM on July 16, 2019:

In commit "Have importaddress use ImportScripts and ImportScriptPubKeys" (d2b7db13b92391c87adf6db862593b311c7cbdee):

Maybe call ImportScripts({redeem_script}) here to avoid needing to share scripts variable and have its value change between the two calls.

achow101 commented at 10:38 PM on July 16, 2019:

I don't see the benefit of doing that.

in src/wallet/rpcdump.cpp:1249 in ace5d08203 outdated

1257 | @@ -1258,7 +1258,7 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con
1258 |  
1259 |          // All good, time to import
1260 |          pwallet->MarkDirty();
1261 | -        if (!pwallet->ImportScripts(import_data.import_scripts)) {
1262 | +        if (!pwallet->ImportScripts(import_data.import_scripts, timestamp)) {

ryanofsky commented at 8:00 PM on July 16, 2019:

In commit "Optionally allow ImportScripts to set script creation timestamp" (ace5d082034cbaa52f596689433381a2226a21dc)

Can you clairfy in the commit message whether this is a change in behavior for the importmulti RPC. Seems like a minor bugfix?

achow101 commented at 11:10 PM on July 16, 2019:

Done.

in src/wallet/rpcdump.cpp:669 in 991ecfdbbb outdated

 666 | -                pwallet->WalletLogPrintf("Skipping import of %s (key already present)\n", EncodeDestination(PKHash(keyid)));
 667 | +
 668 | +            std::map<CKeyID, CKey> privkey_map;
 669 | +            privkey_map.emplace(keyid, key);
 670 | +
 671 | +            if (!pwallet->ImportPrivKeys(privkey_map, time)) {

ryanofsky commented at 8:14 PM on July 16, 2019:

In commit "Have importwallet use ImportPrivKeys and ImportScripts" (991ecfdbbb289bd6e4c50fc63f9201edbca14f70)

IMO, would be clearer to call ImportPrivKeys({keyid, key}}, time) than have the extra map variable.

achow101 commented at 11:10 PM on July 16, 2019:

Done

in src/wallet/rpcdump.cpp:670 in 991ecfdbbb outdated

 667 | +
 668 | +            std::map<CKeyID, CKey> privkey_map;
 669 | +            privkey_map.emplace(keyid, key);
 670 | +
 671 | +            if (!pwallet->ImportPrivKeys(privkey_map, time)) {
 672 | +                pwallet->WalletLogPrintf("Skipping import of %s (key already present or an error occurred)\n", EncodeDestination(PKHash(keyid)));

ryanofsky commented at 8:15 PM on July 16, 2019:

In commit "Have importwallet use ImportPrivKeys and ImportScripts" (991ecfdbbb289bd6e4c50fc63f9201edbca14f70)

Ths doesn't seem right, if HaveKey is true, ImportPrivKeys returns true.

achow101 commented at 10:43 PM on July 16, 2019:

~~No?~~

nvm, looking at a change that I had locally

achow101 commented at 11:10 PM on July 16, 2019:

I've restored fGood and the error messages.

in src/wallet/rpcdump.cpp:674 in 991ecfdbbb outdated

 671 | +            if (!pwallet->ImportPrivKeys(privkey_map, time)) {
 672 | +                pwallet->WalletLogPrintf("Skipping import of %s (key already present or an error occurred)\n", EncodeDestination(PKHash(keyid)));
 673 |                  continue;
 674 |              }
 675 | +
 676 |              pwallet->WalletLogPrintf("Importing %s...\n", EncodeDestination(PKHash(keyid)));

ryanofsky commented at 8:17 PM on July 16, 2019:

In commit "Have importwallet use ImportPrivKeys and ImportScripts" (991ecfdbbb289bd6e4c50fc63f9201edbca14f70)

This print statement should precede the import above...

achow101 commented at 11:10 PM on July 16, 2019:

Fixed

ryanofsky commented at 8:41 PM on July 16, 2019: member

I reviewed the whole PR, and I think it's close, but am unsure about some issues. Details are in code comments, but my main concerns were:

It seems bad that importprivkey might be calling ImportScriptPubKeys instead of SetAddressBook just to set the label.
There might be some unintended changes around updating nCreateTime. In general, commits are unclear about how and whether they are intended to change behavior.
Error handling in the importwallet commit seems wrong

But aside from these specific concerns, the change seem very good and moves in a welcome direction.

achow101 force-pushed on Jul 16, 2019

in src/wallet/rpcdump.cpp:713 in 7a58ad5624 outdated

 694 |                  fGood = false;
 695 |                  continue;
 696 |              }
 697 | -            if (time > 0) {
 698 | -                pwallet->m_script_metadata[id].nCreateTime = time;
 699 | -                nTimeBegin = std::min(nTimeBegin, time);

promag commented at 9:54 AM on July 17, 2019:

7a58ad5624b2ea9c2d2c661446cc6e046a222fca

nTimeBegin is not being updated, the rescan below could miss wallet transactions?

achow101 commented at 3:21 PM on July 17, 2019:

Added the nTimeBegin update back.

in src/wallet/rpcdump.cpp:660 in 7a58ad5624 outdated

 655 | @@ -656,43 +656,34 @@ UniValue importwallet(const JSONRPCRequest& request)
 656 |              CPubKey pubkey = key.GetPubKey();
 657 |              assert(key.VerifyPubKey(pubkey));
 658 |              CKeyID keyid = pubkey.GetID();
 659 | -            if (pwallet->HaveKey(keyid)) {
 660 | -                pwallet->WalletLogPrintf("Skipping import of %s (key already present)\n", EncodeDestination(PKHash(keyid)));
 661 | -                continue;
 662 | -            }
 663 | +
 664 |              pwallet->WalletLogPrintf("Importing %s...\n", EncodeDestination(PKHash(keyid)));

promag commented at 10:01 AM on July 17, 2019:

7a58ad5624b2ea9c2d2c661446cc6e046a222fca

nit, this wouldn't be logged if the key is present.

achow101 commented at 3:22 PM on July 17, 2019:

Added a note in the commit message that this happens.

achow101 force-pushed on Jul 17, 2019

in src/wallet/wallet.cpp:1701 in 8bd426d238 outdated

1684 | @@ -1680,9 +1685,14 @@ bool CWallet::ImportPrivKeys(const std::map<CKeyID, CKey>& privkey_map, const in
1685 |          CPubKey pubkey = key.GetPubKey();
1686 |          const CKeyID& id = entry.first;
1687 |          assert(key.VerifyPubKey(pubkey));
1688 | +        // Skip if we already have the key
1689 | +        if (HaveKey(id)) {
1690 | +            WalletLogPrintf("Already have key with pubkey %s, skipping\n", HexStr(pubkey));
1691 | +            continue;
1692 | +        }
1693 |          mapKeyMetadata[id].nCreateTime = timestamp;

ryanofsky commented at 5:08 PM on July 17, 2019:

In commit "Log when an import is being skipped because we already have it" (8bd426d238a526f8cfa4e8dd567fb3ccadb40cf7)

Note: Beyond adding the log, this commit also changes behavior of ImportPrivKeys to not update the nCreateTime when the key already exists. This is a good change, so just wanted to point it out.

in src/wallet/rpcdump.cpp:191 in 5776e6d72e outdated

 186 | @@ -187,19 +187,15 @@ UniValue importprivkey(const JSONRPCRequest& request)
 187 |                  }
 188 |              }
 189 |  
 190 | -            // Don't throw error in case a key is already there
 191 | -            if (pwallet->HaveKey(vchAddress)) {
 192 | -                return NullUniValue;
 193 | +            // Use timestamp of 1 to scan the whole chain
 194 | +            if (!pwallet->ImportPrivKeys({{vchAddress, key}}, 1)) {

ryanofsky commented at 5:15 PM on July 17, 2019:

In commit "Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys" (5776e6d72e76e03251d346588035fcdb0423440c)

Nice! From ea665e323f76766def6c4f46a46c53e38365d179 to 5776e6d72e76e03251d346588035fcdb0423440c this commit is so much simpler now. The ImportScriptPubKeys stuff was so confusing to me before.

in src/wallet/rpcdump.cpp:668 in 35805ce74a outdated

 671 |              }
 672 | -            pwallet->mapKeyMetadata[keyid].nCreateTime = time;
 673 | -            if (has_label)
 674 | -                pwallet->SetAddressBook(PKHash(keyid), label, "receive");
 675 | +
 676 | +            pwallet->ImportScriptPubKeys(label, {GetScriptForDestination(PKHash(keyid))}, true /* have_solving_data */, has_label /* apply_label */, time /* timestamp */);

ryanofsky commented at 5:08 PM on July 18, 2019:

In commit "Have importwallet use ImportPrivKeys and ImportScripts" (35805ce74a780674c2a04259f98c8888906d7e91)

Any particular reason this this is calling ImportScriptPubKeys to indirectly set the labels instead of just directly calling SetAddressBook like before? This seems to create same confusion around removing watch only scripts, and then appearing to add them again, but then not really adding them due to ismine, that was in a prior version of the importprivkey commit #16301 (review).

Would suggest reverting here and just calling SetAddressBook instead of ImportScriptPubKeys like before. Or adding a comment about what ImportScriptPubKeys is intended to do in this context.

achow101 commented at 12:36 AM on July 19, 2019:

Reverted.

in src/wallet/rpcdump.cpp:684 in 35805ce74a outdated

 692 | +            if (!pwallet->ImportScripts({script}, time)) {
 693 |                  pwallet->WalletLogPrintf("Error importing script %s\n", HexStr(script));
 694 |                  fGood = false;
 695 |                  continue;
 696 |              }
 697 |              if (time > 0) {

ryanofsky commented at 5:19 PM on July 18, 2019:

In commit "Have importwallet use ImportPrivKeys and ImportScripts" (35805ce74a780674c2a04259f98c8888906d7e91)

Interesting there is still a case that treats 0 and 1 key times differently. It might a be nice idea in a followup PR to replace all the 0's and 1's in the code with SKIP_RESCAN = 0, FULL_RESCAN = 1 constants.

achow101 commented at 12:37 AM on July 19, 2019:

I think the use of 0 and 1 is kind of inconsistent and all of the the timestamp stuff needs to be cleaned up. But that should be done in a followup PR.

in src/wallet/wallet.cpp:1727 in 8bd426d238 outdated

1712 | @@ -1703,7 +1713,12 @@ bool CWallet::ImportPubKeys(const std::vector<CKeyID>& ordered_pubkeys, const st
1713 |          }
1714 |          const CPubKey& pubkey = entry->second;
1715 |          CPubKey temp;
1716 | -        if (!GetPubKey(id, temp) && !AddWatchOnlyWithDB(batch, GetScriptForRawPubKey(pubkey), timestamp)) {
1717 | +        if (GetPubKey(id, temp)) {
1718 | +            // Already have pubkey, skipping
1719 | +            WalletLogPrintf("Already have pubkey %s, skipping\n", HexStr(temp));
1720 | +            continue;

ryanofsky commented at 8:13 PM on July 18, 2019:

In commit "Log when an import is being skipped because we already have it" (8bd426d238a526f8cfa4e8dd567fb3ccadb40cf7)

Noting another change in behavior: This will now avoid adding the key to the keypool if add_keypool is true and the public key already exists. Could potentially be documented in the commit message or add_keypool description.

achow101 commented at 12:36 AM on July 19, 2019:

Added a note in the commit message

ryanofsky approved

ryanofsky commented at 8:29 PM on July 18, 2019: member

utACK 35805ce74a780674c2a04259f98c8888906d7e91. Thanks for the previous fixes, and feel free to ignore my questions / comments, but I hope one you'll consider is restoring the direct SetAddressBook call in importwallet instead of calling it indirectly through ImportScriptPubKeys, because it's confusing how ImportScriptPubKeys appears to add watch only scripts.

Log when an import is being skipped because we already have it

Behavior Changes:
* Those pubkeys being imported with add_keypool set and are already in the wallet will no longer be added to the keypool

fae7a5befd

Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys

Behavior changes:
* If we already have the key, it's wpkh script will still be added, although it should already be there

c6a8274247

achow101 force-pushed on Jul 19, 2019

ryanofsky approved

ryanofsky commented at 5:35 PM on July 19, 2019: member

utACK bc8ecc056ffa42c758b04d5bcae32c1354c3fbe1. Only change since last review was reverting the importwallet ImportScriptPubKeys change I felt was confusing, and updating one of the commit log messages to note a change in behavior.

Really nice job with this PR, and thanks for being so responsive!

instagibbs commented at 1:35 AM on July 20, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/16301/commits/bc8ecc056ffa42c758b04d5bcae32c1354c3fbe1

Checked diff from https://github.com/bitcoin/bitcoin/commit/991ecfdbbb289bd6e4c50fc63f9201edbca14f70 , was reversions that were discussed and some logging/error reporting improvements.

MarcoFalke commented at 1:18 PM on July 24, 2019: member

partial ACK up to af6dced944

Some questions:

In commit af6dced944, there seems to be a behavior change that is not mentioned in the commit body: Previously, if the privkey already existed, we'd error out. Now, we'd overwrite the label?
In commit af6dced944. Previously, there was a call to AddCScript(GetScriptForDestination(WitnessV0KeyHash(pubKey.GetId()))); (via LearnAllRelatedScripts). I don't see why this is no longer required or where an equivalent call is made. Mind to explain?

<details><summary>Show signature and timestamp</summary>

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

partial ACK up to af6dced944

Some questions:
* In commit af6dced944, there seems to be a behavior change that is not
  mentioned in the commit body: Previously, if the privkey already existed,
  we'd error out. Now, we'd overwrite the label?

* In commit af6dced944. Previously, there was a call to
  AddCScript(GetScriptForDestination(WitnessV0KeyHash(pubKey.GetId())));
  (via LearnAllRelatedScripts). I don't see why this is no longer required
  or where an equivalent call is made. Mind to explain?
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiFywv+Oc9kV/FPwgRVxvIEVgUQpEE4d/DcPiZ205RzdTEOXh7DxzootgnwZSzC
S8obDJGqDy26JSpCr4R6JevUl2gTYVeKMph9G2hlEHZW4pdiAxAY4/xQ8KvoAwBZ
b+u2ovlq6A8uTCQzPYkJ1KAZHHKVwd32PAYllJshilLpdVo5rqXbDwoR322/XoV2
mEeKCjnHBhQDRY4wFQ5g+2onN1oqf2Rw9fELtXqbBR7SM5EjTF79rTfkwLWmDReu
iK92/AQHfQGSrdbILz9VJ9gJE+OCco7dvi/sqMQmXKk9q3aNAY+zDgnQRvvbKkUX
p/yZrBXH8ExN7Fd89D8sb6L/h/floJM0EMQ8Ccc7cVEou77VHLC08NRiLTP5lTzH
nmmd7Br4LqZOQuVPZ9iZCRbUpu+jGPbM0uIqtYDeqO+elSnepWYTwZlCVEfjCwpX
RSaimg4I08v0cpWicoVQcFAB3DxpU0OCJZZEy4/rS5ZwkxVmyorFSy6+Hvh97oCJ
964kzylV
=2JyL
-----END PGP SIGNATURE-----

Timestamp of file with hash 14e6ca5a72d68fc23e91e6eecdf4294ca99ad0e7ce1478ca529dfbbda4ec2d21 -

</details>

Have importpubkey use CWallet's ImportScriptPubKeys and ImportPubKeys functions

Behavior changes:
* If any scripts for the pubkey were already in the wallet, their timestamps will be set to 1 and label updated

a00d1e5ec5

Have importaddress use ImportScripts and ImportScriptPubKeys

Also removes the now unused ImportAddress and ImportScript from rpcdump.cpp

Behavior changes:
* No errors will be thrown when the script or key already exists in the wallet.
* If the key or script is already in the wallet, their labels will be updated.

94bf156f39

Optionally allow ImportScripts to set script creation timestamp

Behavior changes:
* scripts imported in importmulti that are not explicilty scriptPubKeys will have timestamps set for them

78941da5ba

Have importwallet use ImportPrivKeys and ImportScripts

Behavior changes:
* An "Importing ..." line is logged for every key, even ones that are skipped

40ad2f6a58

achow101 force-pushed on Jul 24, 2019

achow101 commented at 3:44 PM on July 24, 2019: member

* In commit [af6dced](https://github.com/bitcoin/bitcoin/commit/af6dced9446eca4a67429e07b3216ae7f3d08c16), there seems to be a behavior change that is not
  mentioned in the commit body: Previously, if the privkey already existed,
  we'd error out. Now, we'd overwrite the label?

Updated the commit message

* In commit [af6dced](https://github.com/bitcoin/bitcoin/commit/af6dced9446eca4a67429e07b3216ae7f3d08c16). Previously, there was a call to
  AddCScript(GetScriptForDestination(WitnessV0KeyHash(pubKey.GetId())));
  (via LearnAllRelatedScripts). I don't see why this is no longer required
  or where an equivalent call is made. Mind to explain?

ImportPubKeys will add the public key to mapWatchKeys. In doing so, it calls ImplicitlyLearnRelatedKeyScripts which will find that script and add it to mapScripts in memory. Every time this key is loaded, this script will always be found and loaded into memory. The only difference is that that script is not persisted to disk.

ryanofsky approved

ryanofsky commented at 4:38 PM on July 24, 2019: member

utACK 40ad2f6a58228c72c655e3061a19a63640419378. Only change since last review is a tweaked commit message (mentioning label update in importpubkey commit)

MarcoFalke commented at 1:41 PM on July 25, 2019: member

ACK 40ad2f6a58228c72c655e3061a19a63640419378 (checked that behavior changes are mentioned in the commit body)

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash bb77b117482794207771d1571ca4e73ac19a12b52c18a1c489b86f8600c76c0b -

</details>

MarcoFalke added this to the milestone 0.19.0 on Jul 25, 2019

MarcoFalke commented at 9:06 PM on July 25, 2019: member

I've written some tests in #16465

Sjors approved

Sjors commented at 6:21 PM on July 26, 2019: member

ACK 40ad2f6a5. Those extra tests also pass.

MarcoFalke merged this on Jul 26, 2019

MarcoFalke closed this on Jul 26, 2019

MarcoFalke referenced this in commit dbf4f3f86a on Jul 26, 2019

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

konez2k referenced this in commit b55323b771 on Jul 27, 2019

deadalnix referenced this in commit 3c4a3c0cf1 on Jun 7, 2020

deadalnix referenced this in commit 33f713967d on Jun 7, 2020

deadalnix referenced this in commit 7952f06f90 on Jun 7, 2020

deadalnix referenced this in commit f5c67f45cc on Jun 7, 2020

deadalnix referenced this in commit 92fe476dac on Jun 7, 2020

deadalnix referenced this in commit dde85a0c75 on Jun 8, 2020

deadalnix referenced this in commit 89fd897823 on Jun 8, 2020

DrahtBot locked this on Dec 16, 2021