Split some CWallet functions into new LegacyScriptPubKeyMan #17260

achow101 · 2019-10-25T21:11:36Z

Moves key management functions into a new class LegacyScriptPubKeyMan. First two commits are move-only commits which move stuff out of wallet.{h/cpp} and into newly created scriptpubkeyman.{h/cpp}. Third commit changes several things in CWallet to use LegacyScriptPubKeyMan.

First step in the wallet boxes refactor. Note that LegacyScriptPubKeyMan and ScriptPubKeyMan cannot be used standalone yet and are still very much tied into CWallet with both accessing functions within each other. This PR is to help reduce review burden.

ryanofsky

ACK af8780a confirming no changes other than rebase from my pr/keyman branch.

Linter fixes in 626eed8

src/wallet/scriptpubkeyman.cpp

src/wallet/test/wallet_tests.cpp

DrahtBot · 2019-10-25T22:45:39Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17283 (rpc: improve getaddressinfo test coverage, help, code docs by jonatack)
#17246 (wallet: avoid knapsack when there's no change by Sjors)
#17237 (wallet: LearnRelatedScripts only if KeepDestination by promag)
#17219 (wallet: allow transaction without change if keypool is empty by Sjors)
#17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)
#16946 (wallet: include a checksum of encrypted private keys by achow101)
#16910 (wallet: reduce loading time by using unordered maps by achow101)
#16546 ([WIP] External signer support - Wallet Box edition by Sjors)
#16528 ([WIP] Native Descriptor Wallets (take 2) by achow101)
#16224 (gui: Bilingual GUI error messages by hebasto)
#15931 (Remove GetDepthInMainChain dependency on locked chain interface by ariard)
#15761 (Replace -upgradewallet startup option with upgradewallet RPC by achow101)
#15294 ([moveonly] wallet: Extract RipeMd160 by Empact)
#14942 (wallet: Make scan / abort status private to CWallet by Empact)
#14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)
#14384 (Fire TransactionRemovedFromMempool callbacks from mempool by l2a5b1)
#14144 (Refactoring: Clarify code using encrypted_batch in CWallet by domob1812)
#10785 (Serialization improvements by sipa)

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.

Start moving wallet and ismine code to scriptpubkeyman.h, scriptpubkeyman.cpp The easiest way to review this commit is to run: git log -p -n1 --color-moved=dimmed_zebra And check that everything is a move (other than includes and copyrights comments). This commit is move-only and doesn't change code or affect behavior.

This moves CWallet members and methods dealing with keys to a new LegacyScriptPubKeyMan class, and updates calling code to reference the new class instead of CWallet. Most of the changes are simple text replacements and variable substitutions easily verified with: git log -p -n1 -U0 --word-diff-regex=. The only nontrivial chunk of code added is the new LegacyScriptPubKeyMan class declaration, but this code isn't new and is just selectively copied and moved from the previous CWallet class declaration. This can be verified with: git log -p -n1 --color-moved=dimmed_zebra src/wallet/scriptpubkeyman.h src/wallet/wallet.h or git diff HEAD~1:src/wallet/wallet.h HEAD:src/wallet/scriptpubkeyman.h This commit does not change behavior.

Sjors · 2019-10-26T08:44:59Z

The code at 1c49a20 matches the first three commits in #17261 modulo the linter changes and extra lock discussed above. I in turn verified that #17261 matches what I previously reviewed.

I'll review this PR on its own merits later.

Sjors

Code review ACK f201ba5.

You could add a seperate commit that moves isminetype IsMine(const CWallet& keystore, const CTxDestination& dest) from IsMine.cpp to wallet.cpp, because that exception to the move from IsMine to scriptpubkeyman is a bit hard to spot now.

src/wallet/rpcdump.cpp

Sjors · 2019-10-26T17:27:23Z

src/wallet/wallet.cpp

-    }
-    return ret;
-}
+std::vector<CKeyID> GetAffectedKeys(const CScript& spk, const SigningProvider& provider);


It's a bit weird to have a definition in a cpp file. On a local branch I was able to move this to scriptpubkeyman.h in the second commit, courtesy of circular includes.

In commit "MOVEONLY: Move key handling code out of wallet to keyman file" (6702048):

re: #17260 (comment)

It's a bit weird to have a definition in a cpp file. On a local branch I was able to move this to scriptpubkeyman.h in the second commit, courtesy of circular includes.

There's a later commit that moves this declaration at the same time as moving the call: 8e743b9, so no need to necessarily move it here, even though it is a reasonable suggestion.

ryanofsky

Thanks for reviewing!

src/wallet/rpcdump.cpp

ryanofsky · 2019-10-26T18:45:49Z

src/wallet/wallet.cpp

-    }
-    return ret;
-}
+std::vector<CKeyID> GetAffectedKeys(const CScript& spk, const SigningProvider& provider);


In commit "MOVEONLY: Move key handling code out of wallet to keyman file" (6702048):

re: #17260 (comment)

It's a bit weird to have a definition in a cpp file. On a local branch I was able to move this to scriptpubkeyman.h in the second commit, courtesy of circular includes.

There's a later commit that moves this declaration at the same time as moving the call: 8e743b9, so no need to necessarily move it here, even though it is a reasonable suggestion.

promag

Code review ACK f201ba5.

promag · 2019-10-28T11:01:12Z

src/wallet/rpcdump.cpp

@@ -138,6 +138,11 @@ UniValue importprivkey(const JSONRPCRequest& request)
        throw JSONRPCError(RPC_WALLET_ERROR, "Cannot import private keys to a wallet with private keys disabled");
    }

+    LegacyScriptPubKeyMan* spk_man = pwallet->GetLegacyScriptPubKeyMan();
+    if (!spk_man) {
+        throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command");


nit, could add some helper to do this repeated check (currently 9 times), like GetLegacyScriptPubKeyMan(pwallet)?

re: #17260 (comment)

nit, could add some helper to do this repeated check (currently 9 times), like GetLegacyScriptPubKeyMan(pwallet)?

Would be a nice followup. My preference would be to do it in a later PR though to avoid delay on this one, since this PR is pretty big and already has some acks.

Agreed for follow up.

ryanofsky

Code review ACK f201ba5

ryanofsky · 2019-10-28T20:27:55Z

src/wallet/rpcdump.cpp

@@ -138,6 +138,11 @@ UniValue importprivkey(const JSONRPCRequest& request)
        throw JSONRPCError(RPC_WALLET_ERROR, "Cannot import private keys to a wallet with private keys disabled");
    }

+    LegacyScriptPubKeyMan* spk_man = pwallet->GetLegacyScriptPubKeyMan();
+    if (!spk_man) {
+        throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command");


re: #17260 (comment)

nit, could add some helper to do this repeated check (currently 9 times), like GetLegacyScriptPubKeyMan(pwallet)?

Would be a nice followup. My preference would be to do it in a later PR though to avoid delay on this one, since this PR is pretty big and already has some acks.

maflcko

Looks good. Just some questions about segmentation faults, which shouldn't be an issue because the pointers are never null in the current code.

ACK f201ba5

Show signature and timestamp

Signature:

Timestamp of file with hash dc73b9d8ac5a486833fc09ba84ab60c9eb65828df3f5ec1320ebd5ff54f522da -

maflcko · 2019-10-29T11:50:31Z

src/wallet/scriptpubkeyman.cpp

 {
-    AssertLockHeld(cs_wallet);


Why is this removed?

maflcko · 2019-10-29T11:55:40Z

src/wallet/scriptpubkeyman.h

+// It provides access to things that are part of the entire wallet and not specific to a ScriptPubKeyMan such as
+// wallet flags, wallet version, encryption keys, encryption status, and the database itself. This allows a
+// ScriptPubKeyMan to have callbacks into CWallet without causing a circular dependency.
+// WalletStorage should be the same for all ScriptPubKeyMans.


in commit f201ba5:

Could clarify that this is "for all ScriptPubKeyMans of a wallet" and not ("for all ScriptPubKeyMans of all wallets")

maflcko · 2019-10-29T11:59:15Z

src/wallet/wallet.cpp

+void CWallet::UpgradeKeyMetadata()
+{
+    AssertLockHeld(m_spk_man->cs_wallet);
+    if (m_spk_man) {


How is this supposed to work? The pointer was already dereferenced in the line above

maflcko · 2019-10-29T12:02:04Z

src/wallet/wallet.cpp

@@ -554,11 +562,11 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
        Unlock(strWalletPassphrase);

        // if we are using HD, replace the HD seed with a new one
-        if (IsHDEnabled()) {
-            SetHDSeed(GenerateNewSeed());
+        if (m_spk_man->IsHDEnabled()) {


Why is a nullptr check required above but not here?

maflcko · 2019-10-29T12:07:42Z

src/wallet/wallet.cpp

-                if (!ProduceSignature(*this, MutableTransactionSignatureCreator(&txNew, nIn, coin.txout.nValue, SIGHASH_ALL), scriptPubKey, sigdata))
+                const SigningProvider* provider = GetSigningProvider();
+                if (!provider) {
+                    return false;


Why is this not setting the error string?

…yMan f201ba5 Refactor: Split up CWallet and LegacyScriptPubKeyMan and classes (Andrew Chow) 6702048 MOVEONLY: Move key handling code out of wallet to keyman file (Andrew Chow) ab053ec Move wallet enums to walletutil.h (Andrew Chow) Pull request description: Moves key management functions into a new class LegacyScriptPubKeyMan. First two commits are move-only commits which move stuff out of wallet.{h/cpp} and into newly created scriptpubkeyman.{h/cpp}. Third commit changes several things in CWallet to use LegacyScriptPubKeyMan. First step in the wallet boxes refactor. Note that LegacyScriptPubKeyMan and ScriptPubKeyMan cannot be used standalone yet and are still very much tied into CWallet with both accessing functions within each other. This PR is to help reduce review burden. ACKs for top commit: Sjors: Code review ACK f201ba5. promag: Code review ACK f201ba5. ryanofsky: Code review ACK f201ba5 MarcoFalke: ACK f201ba5 Tree-SHA512: bdc0d8595a06233fe003afcf968a38e0e8cc584a6a89c5bcd05309ac29dca852391802d46763ef81a108d146d0f40c79ea5438e87234ed12b4b8360c9aec94c0

Suggested by João Barbosa <joao.paulo.barbosa@gmail.com> bitcoin#17260 (comment)

Suggestion from MarcoFalke <falke.marco@gmail.com> bitcoin#17260 (comment)

Suggested by MarcoFalke <falke.marco@gmail.com> bitcoin#17260 (comment)

Suggestion from MarcoFalke <falke.marco@gmail.com> bitcoin#17260 (comment)

Suggested by MarcoFalke <falke.marco@gmail.com> bitcoin#17260 (comment)

Suggested by MarcoFalke <falke.marco@gmail.com> bitcoin#17260 (comment) The same check was also added in the followup commit "Refactor: Move SetupGeneration code out of CWallet" 9727a4d from bitcoin#17261, but it's safer to start checking now without waiting for that followup.

53fe0b7 Fix missing strFailReason in CreateTransaction (Russell Yanofsky) 4b28a05 Fix misplaced AssertLockHeld (Russell Yanofsky) 2632b1f doc: Clarify WalletStorage / Wallet relation (Russell Yanofsky) 628d11b Add back mistakenly removed AssertLockHeld (Russell Yanofsky) 52cf68f Refactor: Add GetLegacyScriptPubKeyMan helper (Russell Yanofsky) Pull request description: This PR implements suggested code cleanups from #17260 review comments ACKs for top commit: Sjors: ACK 53fe0b7 achow101: ACK 53fe0b7 MarcoFalke: ACK 53fe0b7 Tree-SHA512: a577b96cb21a9aa7185d7d900e4db0665c302adcd12097957b9d8e838a8548c7de8f901bcb83e7c46d231b841221345c9264f5e29ed069f3d9236896430f959b

53fe0b7 Fix missing strFailReason in CreateTransaction (Russell Yanofsky) 4b28a05 Fix misplaced AssertLockHeld (Russell Yanofsky) 2632b1f doc: Clarify WalletStorage / Wallet relation (Russell Yanofsky) 628d11b Add back mistakenly removed AssertLockHeld (Russell Yanofsky) 52cf68f Refactor: Add GetLegacyScriptPubKeyMan helper (Russell Yanofsky) Pull request description: This PR implements suggested code cleanups from bitcoin#17260 review comments ACKs for top commit: Sjors: ACK 53fe0b7 achow101: ACK 53fe0b7 MarcoFalke: ACK 53fe0b7 Tree-SHA512: a577b96cb21a9aa7185d7d900e4db0665c302adcd12097957b9d8e838a8548c7de8f901bcb83e7c46d231b841221345c9264f5e29ed069f3d9236896430f959b

maflcko · 2019-11-02T17:38:02Z

This had the nice side effect of cutting peak memory use during compilation:

https://codespeed.bitcoinperf.com/timeline/?exe=3%2C4%2C2%2C1%2C5&base=1%2B23&ben=build.make.1.clang.mem-usage&env=1&revs=10&equid=off&quarts=on&extr=on

https://codespeed.bitcoinperf.com/timeline/?exe=3%2C4%2C2%2C1%2C5&base=1%2B23&ben=build.make.1.gcc.mem-usage&env=1&revs=10&equid=off&quarts=on&extr=on

JeremyRubin · 2019-11-24T05:12:19Z

So I might just be being lazy...

But can you do a bit more documentation on this one? This breaks some of my outstanding work (not PR'd yet) and I'd like to better understand where I should be making this functionality live...

JeremyRubin · 2019-11-24T05:19:46Z

I guess mostly it's just stuff moving into scriptpubkeyman? But do I need to replicate the logic elsewhere now?

achow101 · 2019-11-24T07:04:30Z

@JeremyRubin Have you read https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes

Basically a bunch of things are moving into LegacyScriptPubKeyMan, and some of those that were moved become generic interfaces in CWallet which call the appropriate ScriptPubKeyMan.

Summary: bitcoin/bitcoin@ab053ec --- Depends on D7118 Partial backport of Core [[bitcoin/bitcoin#17260 | PR17260]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6998

…o keyman file Summary: Start moving wallet and ismine code to scriptpubkeyman.h, scriptpubkeyman.cpp The easiest way to review this commit is to run: git log -p -n1 --color-moved=dimmed_zebra And check that everything is a move (other than includes and copyrights comments). This commit is move-only and doesn't change code or affect behavior. bitcoin/bitcoin@6702048 --- Depends on D6998 Partial backport of Core [[bitcoin/bitcoin#17260 | PR17260]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7006

…yMan and classes Summary: This moves CWallet members and methods dealing with keys to a new LegacyScriptPubKeyMan class, and updates calling code to reference the new class instead of CWallet. Most of the changes are simple text replacements and variable substitutions easily verified with: git log -p -n1 -U0 --word-diff-regex=. The only nontrivial chunk of code added is the new LegacyScriptPubKeyMan class declaration, but this code isn't new and is just selectively copied and moved from the previous CWallet class declaration. This can be verified with: git log -p -n1 --color-moved=dimmed_zebra src/wallet/scriptpubkeyman.h src/wallet/wallet.h or git diff HEAD~1:src/wallet/wallet.h HEAD:src/wallet/scriptpubkeyman.h This commit does not change behavior. --- bitcoin/bitcoin@f201ba5 Depends on D7006 Concludes backport of Core [[bitcoin/bitcoin#17260 | PR17260]] NOTE: some changes to wallet/ismine_tests.cpp should have been made in D6663, but due to PRs being done out of order I missed that many tests were moved into it from test/script_standard_tests.cpp. this diff fixes that oversight. Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7119

Summary: Fix missing strFailReason in CreateTransaction (Russell Yanofsky) Fix misplaced AssertLockHeld (Russell Yanofsky) doc: Clarify WalletStorage / Wallet relation (Russell Yanofsky) Add back mistakenly removed AssertLockHeld (Russell Yanofsky) Refactor: Add GetLegacyScriptPubKeyMan helper (Russell Yanofsky) Pull request description: This PR implements suggested code cleanups from bitcoin/bitcoin#17260 review comments --- https://github.com/bitcoin/bitcoin/pull/17300/files Depends on D7119 Backport of Core [[bitcoin/bitcoin#17300 | PR17300]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7128

53fe0b7 Fix missing strFailReason in CreateTransaction (Russell Yanofsky) 4b28a05 Fix misplaced AssertLockHeld (Russell Yanofsky) 2632b1f doc: Clarify WalletStorage / Wallet relation (Russell Yanofsky) 628d11b Add back mistakenly removed AssertLockHeld (Russell Yanofsky) 52cf68f Refactor: Add GetLegacyScriptPubKeyMan helper (Russell Yanofsky) Pull request description: This PR implements suggested code cleanups from bitcoin#17260 review comments ACKs for top commit: Sjors: ACK 53fe0b7 achow101: ACK 53fe0b7 MarcoFalke: ACK 53fe0b7 Tree-SHA512: a577b96cb21a9aa7185d7d900e4db0665c302adcd12097957b9d8e838a8548c7de8f901bcb83e7c46d231b841221345c9264f5e29ed069f3d9236896430f959b

Suggested by João Barbosa <joao.paulo.barbosa@gmail.com> bitcoin/bitcoin#17260 (comment)

Suggestion from MarcoFalke <falke.marco@gmail.com> bitcoin/bitcoin#17260 (comment)

Suggested by MarcoFalke <falke.marco@gmail.com> bitcoin/bitcoin#17260 (comment)

Suggestion from MarcoFalke <falke.marco@gmail.com> bitcoin/bitcoin#17260 (comment)

Suggested by MarcoFalke <falke.marco@gmail.com> bitcoin/bitcoin#17260 (comment)

Suggestion from MarcoFalke <falke.marco@gmail.com> bitcoin/bitcoin#17260 (comment)

Suggested by MarcoFalke <falke.marco@gmail.com> bitcoin/bitcoin#17260 (comment)

fanquake added Wallet Refactoring labels Oct 25, 2019

achow101 force-pushed the wallet-box-pr-1 branch 3 times, most recently from e30f0a2 to 194227b Compare October 25, 2019 21:37

ryanofsky approved these changes Oct 25, 2019

View reviewed changes

ryanofsky reviewed Oct 25, 2019

View reviewed changes

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

src/wallet/test/wallet_tests.cpp Outdated Show resolved Hide resolved

achow101 force-pushed the wallet-box-pr-1 branch 2 times, most recently from 71588b2 to 3583a11 Compare October 25, 2019 22:57

achow101 added 3 commits October 25, 2019 19:20

Move wallet enums to walletutil.h

ab053ec

achow101 force-pushed the wallet-box-pr-1 branch from 3583a11 to 1c49a20 Compare October 25, 2019 23:22

achow101 mentioned this pull request Oct 26, 2019

Make ScriptPubKeyMan an actual interface and the wallet to have multiple #17261

Merged

achow101 force-pushed the wallet-box-pr-1 branch from 1c49a20 to f201ba5 Compare October 26, 2019 16:25

Sjors approved these changes Oct 26, 2019

View reviewed changes

src/wallet/rpcdump.cpp Show resolved Hide resolved

Sjors reviewed Oct 26, 2019

View reviewed changes

ryanofsky reviewed Oct 26, 2019

View reviewed changes

promag reviewed Oct 28, 2019

View reviewed changes

ryanofsky approved these changes Oct 28, 2019

View reviewed changes

maflcko reviewed Oct 29, 2019

View reviewed changes

maflcko merged commit f201ba5 into bitcoin:master Oct 29, 2019

hebasto mentioned this pull request Oct 29, 2019

gui: Bilingual GUI error messages #16224

Merged

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Oct 29, 2019

Refactor: Add GetLegacyScriptPubKeyMan helper

52cf68f

Suggested by João Barbosa <joao.paulo.barbosa@gmail.com> bitcoin#17260 (comment)

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Oct 29, 2019

Add back mistakenly removed AssertLockHeld

628d11b

Suggestion from MarcoFalke <falke.marco@gmail.com> bitcoin#17260 (comment)

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Oct 29, 2019

doc: Clarify WalletStorage / Wallet relation

2632b1f

Suggested by MarcoFalke <falke.marco@gmail.com> bitcoin#17260 (comment)

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Oct 29, 2019

Fix misplaced AssertLockHeld

4b28a05

Suggestion from MarcoFalke <falke.marco@gmail.com> bitcoin#17260 (comment)

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Oct 29, 2019

Fix missing strFailReason in CreateTransaction

53fe0b7

Suggested by MarcoFalke <falke.marco@gmail.com> bitcoin#17260 (comment)

ryanofsky mentioned this pull request Oct 29, 2019

LegacyScriptPubKeyMan code cleanups #17300

Merged

Empact mentioned this pull request Nov 5, 2019

[moveonly] Extract CWallet::CompactDatabase #17386

Closed

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

silence48 pushed a commit to FantasyGold/FantasyGold-Core that referenced this pull request Nov 14, 2020

Refactor: Add GetLegacyScriptPubKeyMan helper

1bdcc4b

Suggested by João Barbosa <joao.paulo.barbosa@gmail.com> bitcoin/bitcoin#17260 (comment)

silence48 pushed a commit to FantasyGold/FantasyGold-Core that referenced this pull request Nov 14, 2020

Add back mistakenly removed AssertLockHeld

874227c

Suggestion from MarcoFalke <falke.marco@gmail.com> bitcoin/bitcoin#17260 (comment)

silence48 pushed a commit to FantasyGold/FantasyGold-Core that referenced this pull request Nov 14, 2020

Fix misplaced AssertLockHeld

afe605b

Suggestion from MarcoFalke <falke.marco@gmail.com> bitcoin/bitcoin#17260 (comment)

silence48 pushed a commit to FantasyGold/FantasyGold-Core that referenced this pull request Nov 14, 2020

Fix missing strFailReason in CreateTransaction

8fc9692

Suggested by MarcoFalke <falke.marco@gmail.com> bitcoin/bitcoin#17260 (comment)

silence48 pushed a commit to FantasyGold/FantasyGold-Core that referenced this pull request Nov 15, 2020

Add back mistakenly removed AssertLockHeld

dac19f3

Suggestion from MarcoFalke <falke.marco@gmail.com> bitcoin/bitcoin#17260 (comment)

silence48 pushed a commit to FantasyGold/FantasyGold-Core that referenced this pull request Nov 15, 2020

doc: Clarify WalletStorage / Wallet relation

6fb9cd1

Suggested by MarcoFalke <falke.marco@gmail.com> bitcoin/bitcoin#17260 (comment)

silence48 pushed a commit to FantasyGold/FantasyGold-Core that referenced this pull request Nov 15, 2020

Fix misplaced AssertLockHeld

fd8e538

Suggestion from MarcoFalke <falke.marco@gmail.com> bitcoin/bitcoin#17260 (comment)

silence48 pushed a commit to FantasyGold/FantasyGold-Core that referenced this pull request Nov 15, 2020

Fix missing strFailReason in CreateTransaction

c4276b3

Suggested by MarcoFalke <falke.marco@gmail.com> bitcoin/bitcoin#17260 (comment)

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split some CWallet functions into new LegacyScriptPubKeyMan #17260

Split some CWallet functions into new LegacyScriptPubKeyMan #17260

achow101 commented Oct 25, 2019

ryanofsky left a comment

DrahtBot commented Oct 25, 2019 •

edited

Loading

Sjors commented Oct 26, 2019

Sjors left a comment •

edited

Loading

Sjors Oct 26, 2019

ryanofsky Oct 26, 2019 •

edited

Loading

ryanofsky left a comment

ryanofsky Oct 26, 2019 •

edited

Loading

promag left a comment

promag Oct 28, 2019

ryanofsky Oct 28, 2019

achow101 Oct 28, 2019

ryanofsky left a comment

ryanofsky Oct 28, 2019

maflcko left a comment

maflcko Oct 29, 2019

maflcko Oct 29, 2019

maflcko Oct 29, 2019

maflcko Oct 29, 2019

maflcko Oct 29, 2019

maflcko commented Nov 2, 2019

JeremyRubin commented Nov 24, 2019

JeremyRubin commented Nov 24, 2019

achow101 commented Nov 24, 2019

Split some CWallet functions into new LegacyScriptPubKeyMan #17260

Split some CWallet functions into new LegacyScriptPubKeyMan #17260

Conversation

achow101 commented Oct 25, 2019

ryanofsky left a comment

Choose a reason for hiding this comment

DrahtBot commented Oct 25, 2019 • edited Loading

Conflicts

Sjors commented Oct 26, 2019

Sjors left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky Oct 26, 2019 • edited Loading

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Oct 26, 2019 • edited Loading

Choose a reason for hiding this comment

promag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko commented Nov 2, 2019

JeremyRubin commented Nov 24, 2019

JeremyRubin commented Nov 24, 2019

achow101 commented Nov 24, 2019

DrahtBot commented Oct 25, 2019 •

edited

Loading

Sjors left a comment •

edited

Loading

ryanofsky Oct 26, 2019 •

edited

Loading

ryanofsky Oct 26, 2019 •

edited

Loading