BACKPORT NOTICE
fixup psbt. all missing changes belongs to src/wallet/scriptpubkeyman.h/cpp ----- they are related to descriptor wallet!
-------------------
931dd4760855e036c176a23ec2de367c460e4243 Make lint-spelling.py happy (Glenn Willen)
11a0ffb29d1b4dcc55c8826873f340ab4196af21 [gui] Load PSBT from clipboard (Glenn Willen)
a6cb0b0c29d327d01aebb98b0504f317eb19c3dc [gui] PSBT Operations Dialog (sign & broadcast) (Glenn Willen)
5dd0c03ffa3aeaa69d8a3a716f902f450d5eaaec FillPSBT: report number of inputs signed (or would sign) (Glenn Willen)
9e7b23b73387600d175aff8bd5e6624dd51f86e7 Improve TransactionErrorString messages. (Glenn Willen)
Pull request description:
Add a "PSBT Operations" dialog, reached from the "Load PSBT..." menu item, giving options to sign or broadcast the loaded PSBT as appropriate, as well as copying the result to the clipboard or saving it to a file.
This is based on Sjors' #17509, and depends on that PR going in first. (It effectively replaces the small "load PSBT" dialog from that PR with a more feature-rich one.)
Some notes:
* The way I display status information is maybe unusual (a status bar, rather than messageboxes.) I think it's helpful to have the information in it be persistent rather than transitory. But if people dislike it, I would probably move the "current state of the transaction" info to the top line of the main label, and the "what action just happened, and did it succeed" info into a messagebox.
* I don't really know much about the translation/localization stuff. I put tr() in all the places it seemed like it ought to go. I did not attempt to translate the result of TransactionErrorString (which is shared by GUI and non-GUI code); I don't know if that's correct, but it matches the "error messages in logs should be googleable in English" heuristic. I don't know whether there are things I should be doing to reduce translator effort (like minimizing the total number of distinct message strings I use, or something.)
* I don't really know how (if?) automated testing is applied to GUI code. I can make a list of PSBTs exercising all the codepaths for manual testing, if that's the right approach. Input appreciated.
ACKs for top commit:
instagibbs:
tested ACK 931dd47608
Sjors:
re-tACK 931dd4760855e036c176a23ec2de367c460e4243
jb55:
ACK 931dd4760855e036c176a23ec2de367c460e4243
achow101:
ACK 931dd4760855e036c176a23ec2de367c460e4243
Tree-SHA512: ade52471a2242f839a8bd6a1fd231443cc4b43bb9c1de3fb5ace7c5eb59eca99b1f2e9f17dfdb4b08d84d91f5fd65677db1433dd03eef51c7774963ef4e2e74f
d49612f98add29066817b7c808b76c2d728948e5 Make SetMiscWarning() accept bilingual_str argument (Hennadii Stepanov)
d1ae7c0355662481a7d181a0a458284936d53eb1 Make GetWarnings() return bilingual_str (Hennadii Stepanov)
38e33aa481cefbe12c50f344bae190c0d95fb489 refactor: Make GetWarnings() bilingual_str aware internally (Hennadii Stepanov)
Pull request description:
This is one more step for consistent usage of `bilingual_str`.
No new translation messages are defined.
ACKs for top commit:
laanwj:
Code review ACK d49612f98add29066817b7c808b76c2d728948e5
MarcoFalke:
ACK d49612f98add29066817b7c808b76c2d728948e5 🌂
Tree-SHA512: 7413cb94a85291209c182845f6873350bb9e9ce940647d416c462a136603832fec8a63d792341bf634f07629767c78bc206d3a318cf10c7e87241c114c2496e9
a0d0f1c6c3d736bc0ee076b7f27a0ef59fd260bc refactor: Remove Node:: queries from GUI (Hennadii Stepanov)
06d519f0b43ed16252428e935d3aeb5a38f582e0 qt: Add SynchronizationState enum to signal parameter (Hennadii Stepanov)
3c709aa69d5bb5a1564c339a0e6a16bac8f02c98 refactor: Remove Node::getReindex() call from GUI (Hennadii Stepanov)
1dab574edf57ccd6cdf5ec706ac328c62142d7a2 refactor: Pass SynchronizationState enum to GUI (Hennadii Stepanov)
2bec309ad6d0f2543948d64ed26f7d9a903f67e5 refactor: Remove unused bool parameter in RPCNotifyBlockChange() (Hennadii Stepanov)
1df77014d8bb733d7d89e36b28671cb47f436292 refactor: Remove unused bool parameter in BlockNotifyGenesisWait() (Hennadii Stepanov)
Pull request description:
This PR is a followup of #18121 and:
- addresses confusion about GUI notification throttling conditions (**luke-jr**'s [comment](https://github.com/bitcoin/bitcoin/pull/18121#discussion_r378552386), **ryanofsky**'s [comment](https://github.com/bitcoin/bitcoin/pull/18121#discussion_r378975960))
- removes `isInitialBlockDownload()` call from the GUI back to the node (on macOS). See: **ryanofsky**'s [comment](https://github.com/bitcoin/bitcoin/pull/18121#pullrequestreview-357730284)
ACKs for top commit:
jonasschnelli:
Core Review ACK a0d0f1c6c3d736bc0ee076b7f27a0ef59fd260bc (modulo [question](https://github.com/bitcoin/bitcoin/pull/18152#pullrequestreview-414140601)).
ryanofsky:
Code review ACK a0d0f1c6c3d736bc0ee076b7f27a0ef59fd260bc. Only changes since last review were rebase and tweaking SynchronizationState enum declaration as suggested (thanks!)
Tree-SHA512: b6a712a710666e763aeee0d5440de1391a4c6c8f7fa661888773e1ba59e9e0f83654ee384d4edc704031be7eb25616e5eca2a6e26058d3efb7f64c47f9ed7316
7eaf86d3bfc83f2beb3ef449707d5156853126fb trivial: Suggested cleanups to surrounding code (Russell Yanofsky)
b604c5c8b5892842f13dee89ae31812a28ab25d1 wallet: Minimal fix to restore conflicted transaction notifications (Russell Yanofsky)
Pull request description:
This fix is a based on the fix by Antoine Riard (ariard) in https://github.com/bitcoin/bitcoin/pull/18600.
Unlike that PR, which implements some new behavior, this just restores previous wallet notification and status behavior for transactions removed from the mempool because they conflict with transactions in a block. The behavior was accidentally changed in two `CWallet::BlockConnected` updates: a31be09bfd77eed497a8e251d31358e16e2f2eb1 and 7e89994133725125dddbfa8d45484e3b9ed51c6e from https://github.com/bitcoin/bitcoin/pull/16624, causing issue https://github.com/bitcoin/bitcoin/issues/18325.
The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations.
Fixes#18325
Co-authored-by: Antoine Riard (ariard)
ACKs for top commit:
jonatack:
Re-ACK 7eaf86d3bfc83f
ariard:
ACK 7eaf86d, reviewed, built and ran tests.
MarcoFalke:
ACK 7eaf86d3bfc83f2beb3ef449707d5156853126fb 🍡
Tree-SHA512: 9a1efe975969bb522a9dd73c41064a9348887cb67883cd92c6571fd2df4321b9f4568363891abdaae14a3b9b168ef8142e95c373fc04677e46289b251fb84689
faca73000fa8975c28f6be8be01957c1ae94ea62 ci: Install fixed version of clang-format for linters (MarcoFalke)
fa4695da4c69646b58a8fa0b6b30146bb234deb8 build: Sort Makefile.am after renaming file (MarcoFalke)
cccc2784a3bb10fa8e43be7e68207cafb12bd915 scripted-diff: Move ui_interface to the node lib (MarcoFalke)
fa72ca6a9d90d66012765b0043fd819698b94ba8 qt: Remove unused includes (MarcoFalke)
fac96e6450d595fe67168cb7afa7692da6cc9973 wallet: Do not include server symbols (MarcoFalke)
fa0f6c58c1c6d10f04c4e65a424cc51ebca50a8c Revert "Fix link error with --enable-debug" (MarcoFalke)
Pull request description:
This reverts a hacky workaround from commit b83cc0f, which only happens to work due to compiler optimizations. Then, it actually fixes the linker error.
The underlying problem is that the wallet includes symbols from the server (ui_interface), which usually results in linker failures. Though, in this specific case the linker failures have not been observed (unless `-O0`) because our compilers were smart enough to strip unused symbols.
Fix the underlying problem by creating a new header-only with the needed symbol and move ui_interface to node to clarify that this is part of libbitcoin_server.
ACKs for top commit:
Sjors:
ACK faca730
laanwj:
ACK faca73000fa8975c28f6be8be01957c1ae94ea62
hebasto:
re-ACK faca73000fa8975c28f6be8be01957c1ae94ea62, since the [previous](https://github.com/bitcoin/bitcoin/pull/19331#pullrequestreview-434420539) review:
Tree-SHA512: e9731f249425aaea50b6db5fc7622e10078cf006721bb87989cac190a2ff224412f6f8a7dd83efd018835302337611f5839e29e15bef366047ed591cef58dfb4
fa4e088cbac035b8029a10b492849540150d0622 wallet: Mark replaced tx to not be in the mempool anymore (MarcoFalke)
Pull request description:
The wallet does not mark the replaced tx as out-of-mempool. This causes failures in user scripts, because later RPCs may depend on this state change from `bumpfee`.
For example, the following might fail on current master:
```
txid = sendtoaddress(...)
bumpfee(txid)
abandontransaction(txid) # fails because txid is still marked as "in mempool"
```
Fixes#18831
ACKs for top commit:
meshcollider:
utACK fa4e088cbac035b8029a10b492849540150d0622
ryanofsky:
Code review ACK fa4e088cbac035b8029a10b492849540150d0622, and previous ACK faeedff5c87091fd83d2fb2b29eb49c948363f29 is also still valid in case there's a preference for the original fix
Tree-SHA512: 9858f40f5fb5a43a7b584b5c4268b6befa82e6a84583be5206fe721bcb6c255e8d35479d347d0b9aed72703df49887c02b14ab680e8efdd28b90dd6b93d9439a
aaaaad6ac95b402fe18d019d67897ced6b316ee0 scripted-diff: Bump copyright of files changed in 2019 (MarcoFalke)
Pull request description:
ACKs for top commit:
practicalswift:
ACK aaaaad6ac95b402fe18d019d67897ced6b316ee0
promag:
ACK aaaaad6ac95b402fe18d019d67897ced6b316ee0 🎉
fanquake:
ACK aaaaad6ac95b402fe18d019d67897ced6b316ee0 - going to merge this now because the year is over and conflicts are minimal.
Tree-SHA512: 58cb1f53bc4c1395b2766f36fabc7e2332e213780a802762fff0afd59468dad0c3265f553714d761c7a2c44ff90f7dc250f04458f4b2eb8eef8b94f8c9891321
## Issue being fixed or feature implemented
fix: possible assert call if nHeight in CDeterministicMNListDiff is
higher than Tip
Example of new log:
```
2023-09-28T17:35:50Z GetProjectedMNPayeesAtChainTip WARNING pindex is nullptr due to height=914160 chain height=914159
```
instead assert call:
```
...
#6 0x00007ffff7a33b86 in __assert_fail (assertion=0x55555783afd2 "pindex", file=0x5555577f2ed8 "llmq/utils.cpp", line=730,
function=0x5555577f2448 "bool llmq::utils::IsMNRewardReallocationActive(const CBlockIndex*)") at ./assert/assert.c:101
#7 0x0000555555ab7daf in llmq::utils::IsMNRewardReallocationActive (pindex=<optimized out>) at llmq/utils.cpp:730
#8 0x00005555559458ad in CDeterministicMNList::GetProjectedMNPayees (this=this@entry=0x7fffffffc690, pindex=0x0, nCount=<optimized out>, nCount@entry=2147483647)
at evo/deterministicmns.cpp:231
#9 0x000055555594614f in CDeterministicMNList::GetProjectedMNPayeesAtChainTip (this=this@entry=0x7fffffffc690, nCount=nCount@entry=2147483647) at evo/deterministicmns.cpp:216
#10 0x00005555558c9f51 in MasternodeList::updateDIP3List (this=this@entry=0x55555908cfd0) at qt/masternodelist.cpp:194
#11 0x00005555558ca9a0 in MasternodeList::updateDIP3ListScheduled (this=0x55555908cfd0) at qt/masternodelist.cpp:157
#12 0x000055555684a60f in void doActivate<false>(QObject*, int, void**) ()
#13 0x00005555568525b1 in QTimer::timerEvent(QTimerEvent*) ()
#14 0x0000555556844ce5 in QObject::event(QEvent*) ()
#15 0x0000555556ac3252 in QApplicationPrivate::notify_helper(QObject*, QEvent*) ()
#16 0x000055555681e6b8 in QCoreApplication::sendEvent(QObject*, QEvent*) ()
#17 0x000055555686de2a in QTimerInfoList::activateTimers() ()
#18 0x000055555686be84 in QEventDispatcherUNIX::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) ()
#19 0x00005555569bf8a2 in QXcbUnixEventDispatcher::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) ()
#20 0x000055555681caf6 in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) ()
#21 0x0000555556825f8a in QCoreApplication::exec() ()
...
```
## What was done?
ClientModel returns now a pair: MNList and CBlockIndex; so, we always
know the which one has been used even if current chain is switched.
## How Has This Been Tested?
Run on my localhost from `c034ff0c2606142ba3e8894bc74f693b87374e5c` -
aborted with backtrace like above.
With both of commit - no assert more.
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Motivation
CoinJoin's subsystems are initialized by variables and managers that
occupy the global context. The _extent_ to which these subsystems
entrench themselves into the codebase is difficult to assess and moving
them out of the global context forces us to enumerate the subsystems in
the codebase that rely on CoinJoin logic and enumerate the order in
which components are initialized and destroyed.
Keeping this in mind, the scope of this pull request aims to:
* Reduce the amount of CoinJoin-specific entities present in the global
scope
* Make the remaining usage of these entities in the global scope
explicit and easily searchable
## Additional Information
* The initialization of `CCoinJoinClientQueueManager` is dependent on
blocks-only mode being disabled (which can be alternatively interpreted
as enabling the relay of transactions). The same applies to
`CBlockPolicyEstimator`, which `CCoinJoinClientQueueManager` depends.
Therefore, `CCoinJoinClientQueueManager` is only initialized if
transaction relaying is enabled and so is its scheduled maintenance
task. This can be found by looking at `init.cpp`
[here](93f8df1c31/src/init.cpp (L1681-L1683)),
[here](93f8df1c31/src/init.cpp (L2253-L2255))
and
[here](93f8df1c31/src/init.cpp (L2326-L2327)).
For this reason, `CBlockPolicyEstimator` is not a member of `CJContext`
and its usage is fulfilled by passing it as a reference when
initializing the scheduling task.
* `CJClientManager` has not used `CConnman` or `CTxMemPool` as `const`
as existing code that is outside the scope of this PR would cast away
constness, which would be unacceptable. Furthermore, some logical paths
are taken that will grind to a halt if they are stored as `const`.
Examples of such a call chains would be:
* `CJClientManager::DoMaintenance >
CCoinJoinClientManager::DoMaintenance > DoAutomaticDenominating >
CCoinJoinClientSession::DoAutomaticDenominating >
CCoinJoinClientSession::StartNewQueue > CConnman::AddPendingMasternode`
which modifies `CConnman::vPendingMasternodes`, which is non-const
behaviour
* `CJClientManager::DoMaintenance >
CCoinJoinClientManager::DoMaintenance > DoAutomaticDenominating >
CCoinJoin::IsCollateralValid > AcceptToMemoryPool` which adds a
transaction to the memory pool, which is non-const behaviour
* There were cppcheck [linter
failures](https://github.com/dashpay/dash/pull/5337#issuecomment-1685084688)
that seemed to be caused by the usage of `Assert` in
`coinjoin/client.h`. This seems to be resolved by backporting
[bitcoin#24714](https://github.com/bitcoin/bitcoin/pull/24714). (Thanks
@knst!)
* Depends on #5546
---------
Co-authored-by: Kittywhiskers Van Gogh <63189531+kittywhiskers@users.noreply.github.com>
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
## Issue being fixed or feature implemented
Let GUI users control all CJ params (on the fly) without the need to
edit `dash.conf`.
<img width="643" alt="Screenshot 2023-08-27 at 12 29 07"
src="https://github.com/dashpay/dash/assets/1935069/2d90db0d-c7b2-43a9-9f7f-1c4ad9517408">
## What was done?
Add 3 corresponding spin boxes in Options (with a simple sanity check).
I tried my best to come up with the least confusing labels/tooltips for
these, not sure if I'm 100% happy with the result though.
## How Has This Been Tested?
Run qt wallet, play with values and make sure they are saved/loaded/used
in mixing correctly.
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: Konstantin Akimov <knstqq@gmail.com>
Co-authored-by: thephez <thephez@users.noreply.github.com>
fa4632c41714dfaa699bacc6a947d72668a4deef test: Move boost/stdlib includes last (MarcoFalke)
fa488f131fd4f5bab0d01376c5a5013306f1abcd scripted-diff: Bump copyright headers (MarcoFalke)
fac5c373006a9e4bcbb56843bb85f1aca4d87599 scripted-diff: Sort test includes (MarcoFalke)
Pull request description:
When writing tests, often includes need to be added or removed. Currently the list of includes is not sorted, so developers that write tests and have `clang-format` installed will either have an unrelated change (sorting) included in their commit or they will have to manually undo the sort.
This pull preempts both issues by just sorting all includes in one commit.
Please be aware that this is **NOT** a change to policy to enforce clang-format or any other developer guideline or process. Developers are free to use whatever tool they want, see also #18651.
Edit: Also includes a commit to bump the copyright headers, so that the touched files don't need to be touched again for that.
ACKs for top commit:
practicalswift:
ACK fa4632c41714dfaa699bacc6a947d72668a4deef
jonatack:
ACK fa4632c41714dfaa, light review and sanity checks with gcc build and clang fuzz build
Tree-SHA512: 130a8d073a379ba556b1e64104d37c46b671425c0aef0ed725fd60156a95e8dc83fb6f0b5330b2f8152cf5daaf3983b4aca5e75812598f2626c39fd12b88b180
3dc27a15242a22b5301904375e5880372e9b7f4d doc: Add internal interface conventions to developer notes (Russell Yanofsky)
1dca9dc4c772fa0a4ec52c4d88b7cd3d243aea7b refactor: Change createWallet, fillPSBT argument order (Russell Yanofsky)
96dfe5ced64979e51649d20555aa182defc80119 refactor: Change Chain::broadcastTransaction param order (Russell Yanofsky)
6ceb21909ce66b7b4762a855889acd46bb6b77f3 refactor: Rename Chain::Notifications methods to be consistent with other interfaces methods (Russell Yanofsky)
1c2ab1a6d29f2c6c065dae4f4a4e2ad1286311b3 refactor: Rename Node::disconnect methods (Russell Yanofsky)
77e4b0657298c715c835d8d2eb11e173852e6815 refactor: Get rid of Wallet::IsWalletFlagSet method (Russell Yanofsky)
Pull request description:
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
This PR doesn't change behavior at all, it just cleans up code in [`src/interfaces`](https://github.com/bitcoin/bitcoin/tree/master/src/interfaces) to simplify #10102, and [documents](https://github.com/ryanofsky/bitcoin/blob/pr/ipc-conv/doc/developer-notes.md#internal-interface-guidelines) coding conventions there better
ACKs for top commit:
hebasto:
re-ACK 3dc27a15242a22b5301904375e5880372e9b7f4d, the only change since the [previous](https://github.com/bitcoin/bitcoin/pull/18278#pullrequestreview-372582146) review is rebasing.
MarcoFalke:
ACK 3dc27a15242a22b5301904375e5880372e9b7f4d 🕍
Tree-SHA512: 62e6a0f2488e3924e559d2074ed460b92e7a0a5d98eab492221cb20d59d04bbe32aef2a8aeba5e4ea9168cfa91acd5bc973dce6677be0180bd7a919354df53ed
e09c701e0110350f78366fb837308c086b6503c0 scripted-diff: Bump copyright of files changed in 2020 (MarcoFalke)
6cbe6209646db8914b87bf6edbc18c6031a16f1e scripted-diff: Replace CCriticalSection with RecursiveMutex (MarcoFalke)
Pull request description:
`RecursiveMutex` better clarifies that the mutex is recursive, see also the standard library naming: https://en.cppreference.com/w/cpp/thread/recursive_mutex
For that reason, and to avoid different people asking me the same question repeatedly (e.g. https://github.com/bitcoin/bitcoin/pull/15932#pullrequestreview-339175124 ), remove the outdated alias `CCriticalSection` with a scripted-diff
9924bce317b96ab0c57efb99330abd11b6f16b9a [gui] intro: enable pruning by default unless disk is big (Sjors Provoost)
c8de347a9d6c88fe67d77aba6fcce1b7fd66791c [gui] intro: add prune preference (Sjors Provoost)
1bbc49d2078ee53488e214d00eb47462687b05c5 [gui] intro: inform caller if intro was shown (Sjors Provoost)
1957103786f97135f35ababc97efa1b481865eb0 [gui] add explicit prune setter (Sjors Provoost)
1bccf6a52d7fc08d8f605cfb2edc3277ec299c72 [node] add forceSetArg to interface (Sjors Provoost)
Pull request description:
This adds a checkbox to the intro screen to enable pruning from the get go.
If the user has plenty of space, it's unchecked by default:
<img width="671" alt="big" src="https://user-images.githubusercontent.com/10217/63641289-10339000-c6ac-11e9-98d7-caf64dff0da6.png">
If the user has insufficient space it's checked by default:
<img width="897" alt="low" src="https://user-images.githubusercontent.com/10217/63641276-d4002f80-c6ab-11e9-9f5b-a53472f814ff.png">
When the user has barely enough space and is likely to need pruning in the near future, this is shown in yellow and we also check the prune box:
<img width="662" alt="medium" src="https://user-images.githubusercontent.com/10217/63641294-1c1f5200-c6ac-11e9-8ecb-6b69e42b1ece.png">
The cut-off for this 10 GB above `m_assumed_blockchain_size` (`=240` in `chainparams.cpp`).
If the user launches the first time with `-prune=...` then we disable the check box and display the correct size (rounded to GB):
<img width="658" alt="Schermafbeelding 2019-08-24 om 20 23 14" src="https://user-images.githubusercontent.com/10217/63641351-09594d00-c6ad-11e9-94fe-fe5ed562e109.png">
The 2 GB default matches the settings default. The user can't change it in the intro screen, but can change it later. I'm tempted to increase that default to 10 GB, and then have the intro screen reduce it if space is really tight.
Tips for testing:
* move your existing data dir elsewhere
* wipe data dir at every restart (behavior is different if it exists)
* launch with `bitcoin-qt -resetguisettings -lang=en` (there's some space issues in different languages)
* fake your free space by changing `intro.cpp` line 90: `freeBytesAvailable = 5000000000; // 5 GB`
* try both testnet and mainnet, because settings are seperate. In particular note how step 7 in `GuiMain` switches where `QTSettings settings` points to; this had me thoroughly confused on testnet, because I was setting them too early.
ACKs for top commit:
jonasschnelli:
Tested ACK 9924bce317b96ab0c57efb99330abd11b6f16b9a
ryanofsky:
utACK 9924bce317b96ab0c57efb99330abd11b6f16b9a. The changes are very logical, and implement the feature in a clean that way that doesn't add a lot of complication and shouldn't interfere with future improvements. I looked at Luke's branch too, and I think there's also a lot of great stuff there that seems fully compatible with this change.
Tree-SHA512: 9523961451c53aebd347716976bc3a4a398f989dc21e9bbbd357060bd11a8f46c435f068bd421bb31ccb08e55445ef67bc347d8d19a4fb8fde9d6d3f9a3bcbb0
fa37e798b2660d8e44e31c944a257b55aeef5de2 wallet: Replace confusing getAdjustedTime() with GetTime() (MarcoFalke)
Pull request description:
Setting `nTimeReceived` to the adjusted time has several issues:
* `m_best_block_time` is set to the "unadjusted" time, thus a comparison of the two times is like comparing apples to oranges. In the worst case this opens up an attack vector where remote peers can force a premature re-broadcast of wallet txs.
* The RPC documentation for `"timereceived"` doesn't mention that the network adjusted time is used, possibly confusing users when the time reported by RPC is off by a few seconds compared to their local timestamp.
Fix all issues by replacing the call with `GetTime()`. Also a style fix: Use non-narrowing integer conversion in the RPC method.
ACKs for top commit:
theStack:
Code-review ACK fa37e798b2660d8e44e31c944a257b55aeef5de2
shaavan:
crACK fa37e798b2660d8e44e31c944a257b55aeef5de2
Tree-SHA512: 8d020ba400521246b7aed4b6c41319fc70552e8c69e929a5994500375466a9edac02a0ae64b803dbc6695df22276489561a23bd6e030c44c97d288f7b9b2b3fa
3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf [validation] Remove fMissingInputs from AcceptToMemoryPool() (John Newbery)
c428622a5bb1e37b2e6ab2c52791ac05d9271238 [validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders() (John Newbery)
7204c6434b944f6ad51b3c895837729d3aa56eea [validation] Remove useless ret parameter from Invalid() (John Newbery)
1a37de4b3174d19a6d8691ae07e92b32fdfaef11 [validation] Remove error() calls from Invalid() calls (John Newbery)
067981e49246822421a7bcc720491427e1dba8a3 [validation] Tidy Up ValidationResult class (John Newbery)
a27a2957ed9afbe5a96caa5f0f4cbec730d27460 [validation] Add CValidationState subclasses (John Newbery)
Pull request description:
Carries out some remaining tidy-ups remaining after PR 15141:
- split ValidationState into TxValidationState and BlockValidationState (commit from ajtowns)
- various minor code style tidy-ups to the ValidationState class
- remove the useless `ret` parameter from `ValidationState::Invalid()`
- remove the now unused `first_invalid` parameter from `ProcessNewBlockHeaders()`
- remove the `fMissingInputs` parameter from `AcceptToMemoryPool()`, and deal with missing inputs the same way as other errors by using the `TxValidationState` object.
Tip for reviewers (thanks ryanofsky!): The first commit ("[validation] Add CValidationState subclasses" ) is huge and can be easier to start reviewing if you revert the rote, mechanical changes:
Substitute the commit hash of commit "[validation] Add CValidationState subclasses" for <CommitHash> in the commands below.
```sh
git checkout <CommitHash>
git grep -l ValidationState | xargs sed -i 's/BlockValidationState\|TxValidationState/CValidationState/g'
git grep -l ValidationResult | xargs sed -i 's/BlockValidationResult\|TxValidationResult/ValidationInvalidReason/g'
git grep -l MaybePunish | xargs sed -i 's/MaybePunishNode\(ForBlock\|ForTx\)/MaybePunishNode/g'
git diff HEAD^
```
After that it's possible to easily see the mechanical changes with:
```sh
git log -p -n1 -U0 --word-diff-regex=. <CommitHash>
```
ACKs for top commit:
laanwj:
ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf
amitiuttarwar:
code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf. Also built & ran tests locally.
fjahr:
Code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf . Only nit style change and pure virtual destructor added since my last review.
ryanofsky:
Code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf. Just whitespace change and pure virtual destructor added since last review.
Tree-SHA512: 511de1fb380a18bec1944ea82b513b6192df632ee08bb16344a2df3c40811a88f3872f04df24bc93a41643c96c48f376a04551840fd804a961490d6c702c3d36
5c5d0b62648e1b144b7b93c199f45265dac100e5 Add FoundBlock.found member (Russell Yanofsky)
Pull request description:
This change lets IPC serialization code handle FoundBlock arguments more simply and efficiently. Without this change there was no way to determine from a FoundBlock object whether a block was found or not. So in order to correctly implement behavior of leaving FoundBlock output variables unmodified when a block was not found, IPC code would have to read preexisting output variable values from the local process, send them to the remote process, receive output values back from the remote process, and save them to output variables unconditionally. With FoundBlock.found method, the process is simpler. There's no need to read or send preexisting local output variable values, just to read final output values from the remote process and set them conditionally if the block was found.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). The commit was first part of larger PR #10102.
ACKs for top commit:
fjahr:
Code review ACK 5c5d0b62648e1b144b7b93c199f45265dac100e5
theStack:
Concept and code review ACK 5c5d0b62648e1b144b7b93c199f45265dac100e5
jamesob:
ACK 5c5d0b62648e1b144b7b93c199f45265dac100e5 ([`jamesob/ackr/22215.1.ryanofsky.refactor_add_foundblock`](https://github.com/jamesob/bitcoin/tree/ackr/22215.1.ryanofsky.refactor_add_foundblock))
Zero-1729:
crACK 5c5d0b6
Tree-SHA512: d906e1b7100ff72c3aa06d80bd77673887b2db670ebd52dce7c4f6f557a23a1744c6109308228a37fda6c6ea74f05ba0efecff0ef235ab06ea8acd861fbb8675
b7b9f6e4cee262004643e2fe03d56cb47fdbf5c2 Remove p2pEnabled from Chain interface (Antoine Riard)
Pull request description:
RPC server starts in warmup mode, it can't process yet calls, then follows connection manager initialization and finally RPC server get out of warmup mode. RPC calls shouldn't be able to get P2P disabled errors because once we initialize g_connman it's not unset until shutdown, after RPC server has been stopped.
@mzumsande comment in #15713 let me thought that `p2pEnabled` was maybe useless, `g_connman` is always initialized before RPC server is getting out of warmup. These checks against P2P state were introduced in 5b446dd5b1.
ACKs for top commit:
promag:
ACK b7b9f6e4cee262004643e2fe03d56cb47fdbf5c2
jnewbery:
ACK b7b9f6e4cee262004643e2fe03d56cb47fdbf5c2
Tree-SHA512: 4de2b9fc496bf8347ff5cc645848a5a44c8ca7596cd134f17f3088f5f8262d1d88b8e2a052df93e309ec9a81956a808df17a9eb9f10d4f4d693c95d607fe3561
d2774c09cfcc6c5c967d40bb094eabc8c0bdb6bf Clear any input_errors for an input after it is signed (Andrew Chow)
dc174881ad8498a6905ba282a48077bc5c8037a7 Replace GetSigningProvider with GetSolvingProvider (Andrew Chow)
6a9c429084b40356aa36aa67992da35f61c2f6a2 Move direct calls to MessageSign into new SignMessage functions in CWallet and ScriptPubKeyMan (Andrew Chow)
82a30fade70a2a95c2bbeac4aa06dafda600479d Move key and script filling and signing from CWallet::FillPSBT to ScriptPubKeyMan::FillPSBT (Andrew Chow)
3d70dd99f9f74eef70b19ff6f6f850adc0d5ef8f Move FillPSBT to be a member of CWallet (Andrew Chow)
a4af324d15c1ee43c2abd11a304ae18c7ee82eb0 Use CWallet::SignTransaction in CreateTransaction and signrawtransactionwithwallet (Andrew Chow)
f37de927442d3f024926a66c436d59e391c8696a Implement CWallet::SignTransaction using ScriptPubKeyMan::SignTransaction (Andrew Chow)
d999dd588cab0ff479bc7bee8c9fc33880265ec6 Add SignTransaction function to ScriptPubKeyMan and LegacyScriptPubKeyMan (Andrew Chow)
2c52b59d0a44a86d94fee4e437978d822862c542 Refactor rawtransaction's SignTransaction into generic SignTransaction function (Andrew Chow)
Pull request description:
Following #17261, the way to sign transactions, PSBTs, and messages was to use `GetSigningProvider()` and get a `SigningProvider` containing the private keys. However this may not be feasible for future `ScriptPubKeyMan`s, such as for hardware wallets. Instead of exporting a `SigningProvider` containing private keys, we need to pass these things into the `ScriptPubKeyMan` (via `CWallet`) so that they can do whatever is needed internally to sign them. This is largely a refactor as the logic of processing transactions, PSBTs, and messages for is moved into `LegacyScriptPubKeyMan` and `CWallet` instead of being handled by the caller (e.g. `signrawtransaction`).
To help with this, I've refactored the 3(!) implementations of a `SignTransaction()` function into one generic one. This function will be called by `signrawtransactionwithkey` and `LegacyScriptPubKeyMan::SignTransaction()`. `CWallet::CreateTransaction()` is changed to call `CWallet::SignTransaction()` which in turn, calls `LegacyScriptPubKeyMan::SignTransaction()`. Other `ScriptPubKeyMan`s may implement `SignTransaction()` differently.
`FillPSBT()` is moved to be a member function of `CWallet` and the `psbtwallet.cpp/h` files removed. It is further split so that `CWallet` handles filling the UTXOs while the `ScriptPubKeyMan` handles adding keys, derivation paths, scripts, and signatures. In the end `LegacyScriptPubKeyMan::FillPSBT` still calls `SignPSBTInput`, but the `SigningProvider` is internal to `LegacyScriptPubKeyMan`. Other `ScriptPubKeyMan`s may do something different.
A new `SignMessage()` function is added to both `CWallet` and `ScriptPubKeyMan`. Instead of having the caller (i.e. `signmessage` or the sign message dialog) get the private key, hash the message, and sign, `ScriptPubKeyMan` will now handle that (`CWallet` passes through to the `ScriptPubKeyMan`s as it does for many functions). This signing code is thus consolidated into `LegacyScriptPubKeyMan::SignMessage()`, though other `ScriptPubKeyMan`s may implement it differently. Additionally, a `SigningError` enum is introduced for the different errors that we expect to see from `SignMessage()`.
Lastly, `GetSigningProvider()` is renamed to `GetPublicSigningProvider()`. It will now only provide pubkeys, key origins, and scripts. `LegacySigningProvider` has it's `GetKey` and `HaveKey` functions changed to only return false. Future implementations should return `HidingSigningProvider`s where private keys are hidden.
Other things like `dumpprivkey` and `dumpwallet` are not changed because they directly need and access the `LegacyScriptPubKeyMan` so are not relevant to future changes.
ACKs for top commit:
instagibbs:
reACK d2774c09cf
Sjors:
re-utACK d2774c09cfcc6c5c967d40bb094eabc8c0bdb6bf
meshcollider:
re-utACK d2774c09cfcc6c5c967d40bb094eabc8c0bdb6bf
Tree-SHA512: 89c83e7e7e9315e283fae145a2264648a9d7f7ace8f3281cb3f44f0b013c988d67ba4fa9726e50c643c0ed921bdd269adaec984840d11acf4a681f3e8a582cc1
79facb11e92f8b61063f301027dee7c7344eb1be wallet: use constant CWallets in rpcwallet.cpp (Karl-Johan Alm)
d9b0ebc1da8758645f6de24a4a557511ef9b5e36 wallet: make ReserveDestination pwallet ivar const (Karl-Johan Alm)
57c569e4d9779e2263848770e0ba7eab3054a1bf wallet: make BackupWallet() const (Karl-Johan Alm)
df3a818d2a9fe48e656a8ad2da18fab8a1bfd6e3 wallet: make getters const (Karl-Johan Alm)
227b9dd2d6e1914edfec108af6bec5f12d9f6f39 wallet/spkm: make GetOldestKeyPoolTime() const (Karl-Johan Alm)
22d329ad0ed3ed501bd811720be6a2876d1afe4d wallet: use constant CWallets in rpcdump.cpp (Karl-Johan Alm)
7b3587b29db9eaf11718fc09d48817a45a0a429a wallet/db: make IsDummy() const (Karl-Johan Alm)
d366795d180bc52ba750f71f201a6e5e0c40f1b6 wallet/db: make Backup() const (Karl-Johan Alm)
8cd0b86340870d8f359e4ae26880e03ea36818ab wallet: make CanGetAddresses() const (Karl-Johan Alm)
037fa770eb1ed5152b3ef2c5d3fb2a812d3ef944 wallet: make KeypoolCountExternalKeys() const (Karl-Johan Alm)
ddc93557ad0cf8e433df850d38710828ccd99c16 wallet: make CanGenerateKeys() const (Karl-Johan Alm)
dc2d0650fdb69d27fe1b0092555b7841d542a635 make BlockUntilSyncedToCurrentChain() const (Karl-Johan Alm)
Pull request description:
A lot of places refer to `CWallet*`'s as `CWallet * const`, which translates to *"an immutable pointer to a mutable `CWallet` instance"*; this is
1. often not what the author meant, especially as a lot of these places do not at all modify the wallet object, and
2. confusing, as it tends to suggest that this is a proper way to refer to a constant `CWallet` instance.
This PR changes references to wallets to `const CWallet* const` whenever immutability is expected. This should result in no behavioral changes at all, and improved compile-time error checking.
Note from irc:
> <sipa> sounds good to me; this is the sort of change that as long as it compiles, the behavior shouldn't change
> <sipa> though in general it may lead to introducing automatic copying of objects sometimes (e.g. trying to std::move a const object will work, but generally result in a copy rather than an efficient move)
> <sipa> CWallet objects aren't copied or moved though
ACKs for top commit:
laanwj:
ACK 79facb11e92f8b61063f301027dee7c7344eb1be
Empact:
ACK 79facb11e9
promag:
ACK 79facb11e92f8b61063f301027dee7c7344eb1be.
fjahr:
ACK 79facb11e92f8b61063f301027dee7c7344eb1be
Tree-SHA512: 80a80c1a52f0f788d0ccb268b53bc0f46c796643a3c5a22b55bbbde4ffa6c7e347784e5e53b1e488a3b4e14399e31d5be9417ad5b6319c74a462609e9b1a98e8
3f373659d732a5b1e5fdc692a45b2b8179f66bec Refactor: Replace SigningProvider pointers with unique_ptrs (Andrew Chow)
3afe53c4039103670cec5f9cace897ead76e20a8 Cleanup: Drop unused GUI learnRelatedScripts method (Andrew Chow)
e2f02aa59e3402048269362ff692d49a6df35cfd Refactor: Copy CWallet signals and print function to LegacyScriptPubKeyMan (Andrew Chow)
c729afd0a3b74a3943e4c359270beaf3e6ff8a7b Box the wallet: Add multiple keyman maps and loops (Andrew Chow)
4977c30d59e88a3e5ee248144bcc023debcd895b refactor: define a UINT256_ONE global constant (Andrew Chow)
415afcccd3e5583defdb76e3a280f48e98983301 HD Split: Avoid redundant upgrades (Andrew Chow)
01b4511206e399981a77976deb15785d18db46ae Make UpgradeKeyMetadata work only on LegacyScriptPubKeyMan (Andrew Chow)
4a7e43e8460127a40a7895519587399feff3b682 Store p2sh scripts in AddAndGetDestinationForScript (Andrew Chow)
501acb5538008d98abe79288b92040bc186b93f3 Always try to sign for all pubkeys in multisig (Andrew Chow)
81610eddbc57c46ae243f45d73e715d509f53a6c List output types in an array in order to be iterated over (Andrew Chow)
eb81fc3ee58d3e88af36d8091b9e4017a8603b3c Refactor: Allow LegacyScriptPubKeyMan to be null (Andrew Chow)
fadc08ad944cad42e805228cdd58e0332f4d7184 Locking: Lock cs_KeyStore instead of cs_wallet in legacy keyman (Andrew Chow)
f5be479694d4dbaf59eef562d80fbeacb3bb7dc1 wallet: Improve CWallet:MarkDestinationsDirty (João Barbosa)
Pull request description:
Continuation of wallet boxes project.
Actually makes ScriptPubKeyMan an interface which LegacyScriptPubkeyMan. Moves around functions and things from CWallet into LegacyScriptPubKeyMan so that they are actually separate things without circular dependencies.
***
Introducing the `ScriptPubKeyMan` (short for ScriptPubKeyManager) for managing scriptPubKeys and their associated scripts and keys. This functionality is moved over from `CWallet`. Instead, `CWallet` will have a pointer to a `ScriptPubKeyMan` for every possible address type, internal and external. It will fetch the correct `ScriptPubKeyMan` as necessary. When fetching new addresses, it chooses the `ScriptPubKeyMan` based on address type and whether it is change. For signing, it takes the script and asks each `ScriptPubKeyMan` for whether that `ScriptPubKeyMan` considers that script `IsMine`, whether it has that script, or whether it is able to produce a signature for it. If so, the `ScriptPubKeyMan` will provide a `SigningProvider` to the caller which will use that in order to sign.
There is currently one `ScriptPubKeyMan` - the `LegacyScriptPubKeyMan`. Each `CWallet` will have only one `LegacyScriptPubKeyMan` with the pointers for all of the address types and change pointing to this `LegacyScriptPubKeyMan`. It is created when the wallet is loaded and all keys and metadata are loaded into it instead of `CWallet`. The `LegacyScriptPubKeyMan` is primarily made up of all of the key and script management that used to be in `CWallet`. For convenience, `CWallet` has a `GetLegacyScriptPubKeyMan` which will return the `LegacyScriptPubKeyMan` or a `nullptr` if it does not have one (not yet implemented, but callers will check for the `nullptr`). For purposes of signing, `LegacyScriptPubKeyMan`'s `GetSigningProvider` will return itself rather than a separate `SigningProvider`. This will be different for future `ScriptPubKeyMan`s.
The `LegacyScriptPubKeyMan` will also handle the importing and exporting of keys and scripts instead of `CWallet`. As such, a number of RPCs have been limited to work only if a `LegacyScriptPubKeyMan` can be retrieved from the wallet. These RPCs are `sethdseed`, `addmultisigaddress`, `importaddress`, `importprivkey`, `importpubkey`, `importmulti`, `dumpprivkey`, and `dumpwallet`. Other RPCs which relied on the wallet for scripts and keys have been modified in order to take the `SigningProvider` retrieved from the `ScriptPubKeyMan` for a given script.
Overall, these changes should not effect how everything actually works and the user should experience no difference between having this change and not having it. As such, no functional tests were changed, and the only unit tests changed were those that were directly accessing `CWallet` functions that have been removed.
This PR is the last step in the [Wallet Structure Changes](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes).
ACKs for top commit:
instagibbs:
re-utACK 3f373659d7
Sjors:
re-utACK 3f373659d732a5b1e5fdc692a45b2b8179f66bec (it still compiles on macOS after https://github.com/bitcoin/bitcoin/pull/17261#discussion_r370377070)
meshcollider:
Tested re-ACK 3f373659d732a5b1e5fdc692a45b2b8179f66bec
Tree-SHA512: f8e2b8d9efa750b617691e8702d217ec4c33569ec2554a060141d9eb9b9a3a5323e4216938e2485c44625d7a6e0925d40dea1362b3af9857cf08860c2f344716
fafb381af8279b2d2ca768df0bf68d7eb036a2f9 Remove mempool global (MarcoFalke)
fa0359c5b30730744aa8a7cd9ffab79ded91041f Remove mempool global from p2p (MarcoFalke)
eeee1104d78eb59a582ee1709ff4ac2c33ee1190 Remove mempool global from init (MarcoFalke)
Pull request description:
This refactor unlocks some nice potential features, such as, but not limited to:
* Removing the fee estimates global (would avoid slightly fragile workarounds such as #18766)
* Making the mempool optional for a "blocksonly" operation mode
Even absent those features, the new code without the global should be easier to maintain, read and write tests for.
ACKs for top commit:
jnewbery:
utACK fafb381af8279b2d2ca768df0bf68d7eb036a2f9
hebasto:
ACK fafb381af8279b2d2ca768df0bf68d7eb036a2f9, I have reviewed the code and it looks OK, I agree it can be merged.
darosior:
ACK fafb381af8279b2d2ca768df0bf68d7eb036a2f9
Tree-SHA512: a2e696dc377e2e81eaf9c389e6d13dde4a48d81f3538df88f4da502d3012dd61078495140ab5a5854f360a06249fe0e1f6a094c4e006d8b5cc2552a946becf26
e57980b4738c10344baf136de3e050a3cb958ca5 [mempool] Remove NotifyEntryAdded and NotifyEntryRemoved callbacks (John Newbery)
2dd561f36124972d2364f941de9c3417c65f05b6 [validation] Remove pool member from ConnectTrace (John Newbery)
969b65f3f527631ede1a31c7855151e5c5d91f8f [validation] Remove NotifyEntryRemoved callback from ConnectTrace (John Newbery)
5613f9842b4000fed088b8cf7b99674c328d15e1 [validation] Remove conflictedTxs from PerBlockConnectTrace (John Newbery)
cdb893443cc16edf974f099b8485e04b3db1b1d7 [validation interface] Remove vtxConflicted from BlockConnected (John Newbery)
1168394d759b13af68acec6d5bfa04aaa24561f8 [wallet] Notify conflicted transactions in TransactionRemovedFromMempool (John Newbery)
Pull request description:
These boost signals were added in #9371, before we had a `TransactionRemovedFromMempool` method in the validation interface. The `NotifyEntryAdded` callback was used by validation to build a vector of conflicted transactions when connecting a block, which the wallet was notified of in the `BlockConnected` CValidationInterface callback.
Now that we have a `TransactionRemovedFromMempool` callback, we can fire that signal directly from the mempool for conflicted transactions.
Note that #9371 was implemented to ensure `-walletnotify` events were fired for these conflicted transaction. We inadvertently stopped sending these notifications in #16624 (Sep 2019 commit 7e89994). We should probably fix that, but in a different PR.
ACKs for top commit:
jonatack:
Re-ACK e57980b
ryanofsky:
Code review ACK e57980b4738c10344baf136de3e050a3cb958ca5, no code changes since previous review, but helpful new code comments have been added and the PR description is now more clear about where the old code came from
Tree-SHA512: 3bdbaf1ef2731e788462d4756e69c42a1efdcf168691ce1bbfdaa4b7b55ac3c5b1fd4ab7b90bcdec653703600501b4224d252cfc086aef28f9ce0da3b0563a69
fa9ee52556f493e4a896e2570ca1a3102d777d9a doc: Add doxygen comment to IsRBFOptIn (MarcoFalke)
faef4fc9b4990e563022b6ab595cb02c4060c216 Remove mempool global from interfaces (MarcoFalke)
fa831684e54783f6b40533ca218eb7636bdae667 refactor: Add IsRBFOptInEmptyMempool (MarcoFalke)
Pull request description:
The chain interface has an `m_node` member, which has a pointer to the mempool global. Use the pointer instead of the global to prepare the removal of the mempool global. See #19556
ACKs for top commit:
jnewbery:
utACK fa9ee52556
darosior:
ACK fa9ee52
hebasto:
re-ACK fa9ee52556f493e4a896e2570ca1a3102d777d9a, since my [previous](https://github.com/bitcoin/bitcoin/pull/19848#pullrequestreview-482403942) review:
Tree-SHA512: 11b4c1446f0860a743fdaa67f95c52bf0262d0a4f888be0eaf07ee497448965d32be414111bf016bd568f2989cde923430e3a3889e224057b73c499f06de7199
c6dd565c8820aa8a98b190621e10c6e2821a9ecc [gui] watch-only wallet: copy PSBT to clipboard (Sjors Provoost)
39465d545d521e66bb3accfa788aa94bffaf47eb [wallet] add fillPSBT to interface (Sjors Provoost)
848f88920853724511387ca0b7ef652fa14ced71 [gui] send: include watch-only (Sjors Provoost)
40537f090907f81ba885edb7dff1558382976912 [wallet] ListCoins: include watch-only for wallets without private keys (Sjors Provoost)
Pull request description:
For wallets with `WALLET_FLAG_DISABLE_PRIVATE_KEYS` this makes the watch-only balance available on the send screen (including coin selection). Instead of sending a transaction it generates a PSBT.
The user can take this PSBT and process it with [HWI](https://github.com/bitcoin-core/HWI) or put it an SD card for hardware wallets that support that.
The PSBT is copied to the clipboard. This was the easiest approach; we can add a dialog later to display it, as well as an option to save to disk.
ACKs for top commit:
instagibbs:
test and code review ACK c6dd565c88
meshcollider:
re-ACK c6dd565c8820aa8a98b190621e10c6e2821a9ecc
Tree-SHA512: ebc3da0737e33b255ed926191b84569aedb6097d14868662bd5dce726ce3048e86e9a31eba987b10dffe1482b35c21ae1cd595c2caa4634bc4cf78a826a83852
fa193c6b1b7da8f72a399bfddb1497655ce1685c Add missing includes to fix compile errors (MarcoFalke)
fa09ec83f3f23dacb807c6b6393cabf2a984e4ff Remove unused variables (MarcoFalke)
Pull request description:
This is required for #19183, but seems like good cleanup that can go in upfront.
ACKs for top commit:
practicalswift:
ACK fa193c6b1b7da8f72a399bfddb1497655ce1685c -- patch looks correct
hebasto:
ACK fa193c6b1b7da8f72a399bfddb1497655ce1685c, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 79b94e7f7ee3a1a8a8fb2ea1ecdf61f130f8b133a37865894da3dbbbf311979e7d1fc013b923fdd7dbf19a221e0232f664defbdb57aa44e0b8c45bfff3c71dcb
d0dab897afaac0a18aa47d3ce673a4a43a69178a Refactor: Require scriptPubKey to get wallet SigningProvider (Andrew Chow)
4b0c718f8f48c678cbe4575e9a9cf9e62a30f0da Accumulate result UniValue in SignTransaction (Andrew Chow)
Pull request description:
Easier to review ignoring whitespace:
git log -p -n1 -w
This commit does not change behavior. It passes new CScript arguments to
signing functions, but the arguments aren't currently used.
Split from #17261
ACKs for top commit:
instagibbs:
utACK d0dab897af
ryanofsky:
Code review ACK d0dab897afaac0a18aa47d3ce673a4a43a69178a. Thanks for the SignTransaction update. No other changes since last review
Sjors:
Code review ACK d0dab897afaac0a18aa47d3ce673a4a43a69178a
promag:
Code review ACK d0dab897afaac0a18aa47d3ce673a4a43a69178a.
meshcollider:
Code review ACK d0dab897afaac0a18aa47d3ce673a4a43a69178a
Tree-SHA512: c3f52df20fd9d6b3b5aa65562cf5f7dce7b7f44c148b0f988f8b578fce2a28e9b7bf010f5f04bb5bf60f5272b2899f1dbbfb8aee81579c21c9cba559d1d2bb70
