Age | Commit message (Collapse) | Author |
|
|
|
fae86c38bca5c960462e53975314a0749db5d17d util: Remove unused MilliSleep (MarcoFalke)
fa9af06d91e9357e86863781746f0e78a509967e scripted-diff: Replace MilliSleep with UninterruptibleSleep (MarcoFalke)
fa4620be782c2bf6b5ffddf4f671194fdd1536f3 util: Add UnintrruptibleSleep (MarcoFalke)
Pull request description:
We don't use the interruptible feature of boost's sleep anywhere, so replace it with the sleep in `std::thread`
ACKs for top commit:
ajtowns:
ACK fae86c38bca5c960462e53975314a0749db5d17d quick code review
practicalswift:
ACK fae86c38bca5c960462e53975314a0749db5d17d -- patch looks correct
sipa:
Concept and code review ACK fae86c38bca5c960462e53975314a0749db5d17d
fanquake:
ACK fae86c38bca5c960462e53975314a0749db5d17d - note that an instance of `DHAVE_WORKING_BOOST_SLEEP_FOR` was missed in the [linter](https://github.com/bitcoin/bitcoin/blob/master/test/lint/extended-lint-cppcheck.sh#L69), but that can be cleaned up later.
Tree-SHA512: 7c0f8eb197664b9f7d9fe6c472c77d384f11c797c913afc31de4b532e3b4fd9ea6dd174f92062ff9d1ec39b25e0900ca7c597435add87f0f2477d9557204848c
|
|
fa6b061fc118995eec41766519a11bc0dd1a901d rpc: Auto-format RPCResult (MarcoFalke)
fa7d0503d320900e14c4d9bc016d65c7431070bb rpc: Move OuterType enum to header (MarcoFalke)
Pull request description:
This enforces most syntax rules of the RPCResult at compile time (or some at run time during unit and functional tests)
Apart from normalizing the syntax, by separating stylistic formatting from the structure, we could in theory directly generate the html for e.g. https://bitcoincore.org/en/doc/0.19.0/rpc/wallet/importmulti/
ACKs for top commit:
Sjors:
Indeed, re-ACK fa6b061fc118995eec41766519a11bc0dd1a901d
ajtowns:
ACK fa6b061fc118995eec41766519a11bc0dd1a901d -- skimmed code changes and differences to rpc help output
Tree-SHA512: 5b510b3aa0b7c7b9189a48c77593159409069f939145b9a00c5478e894cf65f994d44d633eb7bb7dbea40ee820645a2930976c24772379d96929002b120efa28
|
|
10efc0487c442bccb0e4a9ac29452af1592a3cf2 Templatize ValidationState instead of subclassing (Jeffrey Czyz)
10e85d4adc9b7dbbda63e00195e0a962f51e4d2c Remove ValidationState's constructor (Jeffrey Czyz)
0aed17ef2892478c28cd660e53223c6dd1dc0187 Refactor FormatStateMessage into ValidationState (Jeffrey Czyz)
Pull request description:
This removes boilerplate code in the subclasses which otherwise only
differ by the result type.
The subclassing was introduced in a27a295.
ACKs for top commit:
MarcoFalke:
ACK 10efc0487c442bccb0e4a9ac29452af1592a3cf2 š±
ajtowns:
ACK 10efc0487c442bccb0e4a9ac29452af1592a3cf2 -- looks good to me
jonatack:
ACK 10efc048 code review, build/tests green, nice cleanup
Tree-SHA512: 765dd52dde7d49b9a5c6d99d97c96f4492673e2aed0b0604faa88db0308fa4500a26bf755cca0b896be283874096c215932e1110a2d01dc012cd36a5fce58a42
|
|
c72a11a1a030036eb1fe4472086a9733731961ce test: Add cost_of_change parameter assertions to bnb_search_test (Yancy Ribbens)
Pull request description:
If the `cost_of_change` variable is removed from the method body `SelectCoinsBnB`, there are currently no failing unit tests. This PR adds assertions about the behavior of the `cost_of_change`: If the cost of creating a change output is greater than what's leftover, then consume the output and create no change, otherwise, don't consume the output (no match found).
ACKs for top commit:
achow101:
ACK c72a11a1a030036eb1fe4472086a9733731961ce
Tree-SHA512: 613aa411df5e2911446e0e8bf3309336faaadf2d3c56e7d125b76454e7c6f9e4f5e8f0910dc6222282628e38cd8a4a7c56bb3d36b564a17f396b9b503ecc64c8
|
|
|
|
|
|
5bad7921d0b33b62c0a59a478c2e8c869fc5e3b5 [test] PSBT RPC: check that bip32_derivs are present by default (Sjors Provoost)
29a21c90610aed88b796a7a5900e42e9048b990e [rpc] set default bip32derivs to true for psbt methods (Sjors Provoost)
Pull request description:
In https://github.com/bitcoin/bitcoin/pull/13557#pullrequestreview-135905054 I recommended not including bip32 deriviation by default in PSBTs:
> _Bit of a privacy issue_: let's say person A and B are about to spend from a multisig address, sending everything to person A. Person A gives their address to person B, their wallet wallet creates a PSBT, but doesn't sign it. Wallet A then calls `walletprocesspsbt` which signs it and _spontaneously adds the master_fingerprint and bip32 path_. Same issue with `walletcreatefundedpsbt`.
>
> Adding `bip32_derivs` should probably be opt-in.
In practice I find this default quite annoying because I forget it and end up with a confused hardware wallet.
More importantly, in the multisig example I provided, it's actually essential for the other side to know the derivation details (in addition to an xpub). This allows them to check that change is going to an address you can still co-sign for (because the spending policy is unchanged except for an index).
ACKs for top commit:
instagibbs:
utACK https://github.com/bitcoin/bitcoin/pull/17264/commits/5bad7921d0b33b62c0a59a478c2e8c869fc5e3b5
jonatack:
ACK 5bad7921d0 code review, built, ran tests, inspected/messed around with/pprinted values from the new tests. Thanks for adding the tests.
meshcollider:
utACK 5bad7921d0b33b62c0a59a478c2e8c869fc5e3b5
Tree-SHA512: 22ad71dda96856060a96758c4ae7aafa22d5e9efba30e0c8287c711e7579849bd72593cbc0f41a2e9e8821315d78bda04e848dbb006283b841b2795e2faebcfd
|
|
e193a84fb28068e38d5f54fbfd6208428c5bb655 Refactor message hashing into a utility function (Jeffrey Czyz)
f8f0d9893d7969bdaa870fadb94ec5d0dfa8334d Deduplicate the message signing code (Vasil Dimov)
2ce3447eb1e25ec7aec4b300dabf6c1e394f1906 Deduplicate the message verifying code (Vasil Dimov)
Pull request description:
The message signing and verifying logic was replicated in a few places
in the code. Consolidate in a newly introduced `MessageSign()` and
`MessageVerify()` and add unit tests for them.
ACKs for top commit:
Sjors:
re-ACK e193a84fb28068e38d5f54fbfd6208428c5bb655
achow101:
ACK e193a84fb28068e38d5f54fbfd6208428c5bb655
instagibbs:
utACK https://github.com/bitcoin/bitcoin/pull/17577/commits/e193a84fb28068e38d5f54fbfd6208428c5bb655
meshcollider:
utACK e193a84fb28068e38d5f54fbfd6208428c5bb655
Tree-SHA512: b0e02a7d4623a98c8f8c77627af1725e6df07700de4630c2f75da6beacdf55414c38ba147bc6d2a757491ab07c827dddf93e8632fe600478760e255714ddab88
|
|
|
|
These functions are used to mark/check if a key of our own has been used to spend (and only for avoid-reuse wallets), which has nothing to do with the destination/address itself.
Give them more accurate names to avoid confusion.
-BEGIN VERIFY SCRIPT-
sed -i -e 's/UsedDestination/SpentKey/g' $(git grep -l 'UsedDestination' ./src)
-END VERIFY SCRIPT-
|
|
This is safe because MilliSleep is never executed in a boost::thread,
the only type of thread that is interruptible.
* The RPC server uses std::thread
* The wallet is either executed in an RPC thread or the main thread
* bitcoin-cli, benchmarks and tests are only one thread (the main thread)
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended -e 's/MilliSleep\((\S+)\);/UninterruptibleSleep(std::chrono::milliseconds{\1});/g' $(git grep -l MilliSleep)
-END VERIFY SCRIPT-
|
|
7f1475c7119e8c72bce39a63386a6ca859066b80 rpc: update validateaddress RPCExamples to bech32 (Sebastian Falbesoner)
Pull request description:
Another small step to get rid of legacy addresses in the RPC help texts and by that encourage the use of bech32 addresses by default. The (invalid) address is the same as in the `getaddressinfo` RPC (see 2ee0cb3330ccf70f0540cb42370796e32eff1569, kudos to jonatack!), I don't think it adds any value to have a different example address per RPC.
ACKs for top commit:
fanquake:
ACK 7f1475c7119e8c72bce39a63386a6ca859066b80
MarcoFalke:
ACK 7f1475c7119e8c72bce39a63386a6ca859066b80
Tree-SHA512: 2350f61fa942a9053f9f5c860ea446965dc7209c71c81bdb98a859d03ca23b225ad72c9c506e4a55c8d8988823d9cfbe808c1a452a1eeadb70ab186b146dd4ca
|
|
recognition
a304a3632f0437f4d0f04589a2200e2da91624a7 Revert "Store p2sh scripts in AddAndGetDestinationForScript" (Russell Yanofsky)
eb7d8a5b07e89133a5fb465ad1b793362e7439f7 [test] check for addmultisigaddress regression (Sjors Provoost)
005f8a92ccb5bc10c8daa106d75e1c21390461d3 wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition (Russell Yanofsky)
Pull request description:
Make `LegacyScriptPubKeyMan::CanProvide` method able to recognize p2sh scripts when the redeem script is present in the `mapScripts` map without the p2sh script also having to be added to the `mapScripts` map. This restores behavior prior to #17261, which I think broke backwards compatibility with old wallet files by no longer treating addresses created by `addmultisigaddress` calls before #17261 as solvable.
The reason why tests didn't fail with the CanProvide implementation in #17261 is because of a workaround added in 4a7e43e8460127a40a7895519587399feff3b682 "Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem for new `addmultisigaddress` RPC calls without fixing it for multisig addresses already created in old wallet files.
This change adds a lot of comments and allows reverting commit 4a7e43e8460127a40a7895519587399feff3b682 "Store p2sh scripts in AddAndGetDestinationForScript", so the `AddAndGetDestinationForScript()` function, `CanProvide()` method, and `mapScripts` map should all be more comprehensible
ACKs for top commit:
Sjors:
re-ACK a304a3632f0437f4d0f04589a2200e2da91624a7 (rebase, slight text changes and my test)
achow101:
re-ACK a304a3632f0437f4d0f04589a2200e2da91624a7
meshcollider:
utACK a304a3632f0437f4d0f04589a2200e2da91624a7
Tree-SHA512: 03b625220c49684c376a8062d7646aeba0e5bfe043f977dc7dc357a6754627d594e070e4d458d12d2291888405d94c1dbe08c7787c318374cedd5755e724fb6e
|
|
4e9efac678a9c0ea4e4c7dd956ea036ae6cf17ec test: Check wallet name in -walletnotify script (JoĆ£o Barbosa)
9a5b5ee81f15b1d89cb25ff3e137a672536cdc46 wallet: Replace %w by wallet name in -walletnotify script (JoĆ£o Barbosa)
Pull request description:
Fixes #13237.
ACKs for top commit:
laanwj:
ACK 4e9efac678a9c0ea4e4c7dd956ea036ae6cf17ec
Tree-SHA512: 189dd1c785485f2e974d7c12531851b2a977778b3b954aa95efd527322ba3345924cfd587fb9c90b0fa979202af0ab2d90e53d125fe266a36c94f757e4176203
|
|
names
fad027fb0ce019f31b20861c37e802d4e29e6931 scripted-diff: Add missing spaces in RPCResult, Fix type names (MarcoFalke)
Pull request description:
This makes the rendered diff smaller when the RPCResult is machine generated later on (Previous attempts: #14601 and #14459)
ACKs for top commit:
Sjors:
ACK fad027fb0ce019f31b20861c37e802d4e29e6931
Tree-SHA512: 48afd571b1cd349ca0b29bb444c1c7cda657e07dd96c610d479f931ccd938186aec98e533d0552b5b10afc9a3d7b911359260a49448e8e1106e3647b2c71f3ba
|
|
The logic of signing a message was duplicated in 3 places:
src/qt/signverifymessagedialog.cpp
SignVerifyMessageDialog::on_signMessageButton_SM_clicked()
src/rpc/misc.cpp
signmessagewithprivkey()
src/wallet/rpcwallet.cpp
signmessage()
Move the logic into
src/util/message.cpp
MessageSign()
and call it from all the 3 places.
|
|
The logic of verifying a message was duplicated in 2 places:
src/qt/signverifymessagedialog.cpp
SignVerifyMessageDialog::on_verifyMessageButton_VM_clicked()
src/rpc/misc.cpp
verifymessage()
with the only difference being the result handling. Move the logic into
a dedicated
src/util/message.cpp
MessageVerify()
which returns a set of result codes, call it from the 2 places and just
handle the results differently in the callers.
|
|
25bc17fceb08ee9625c5e09e2579117ec6f7a1c5 refactor: rpc: Remove vector copy from listtransactions (JoĆ£o Barbosa)
Pull request description:
Current approach
- copy accumulated `ret` vector to `arrTmp`
- drop unnecessary elements from `arrTmp`
- reverse `arrTmp`
- clear `ret`
- copy `arrTmp` to the `ret`
New approach
- create a vector from the accumulated `ret` with just the necessary elements already reversed
- copy it to the result
This PR doesn't change behavior.
ACKs for top commit:
ryanofsky:
Code review ACK 25bc17fceb08ee9625c5e09e2579117ec6f7a1c5. Just comment and commit message tweaks since last review
Tree-SHA512: 87906561e3accdbdb0f4a8194cbcd76ea53ae53d0ce135b90bc54a5f77e300b14ef08505e7daf1fe52426f135442a743da5a027416a769bd454922357cebe7c0
|
|
No change in behavior.
|
|
also contains the following changes:
- rpc: factor out example bech32 address for RPCExamples
- doc: update developer notes wrt RPCExamples addresses
(mention the EXAMPLE_ADDRESS constant as an example for an invalid bech32
address suitable for RPCExamples help documentation)
|
|
Make LegacyScriptPubKeyMan::CanProvide method able to recognize p2sh scripts
when the redeem script is present in the mapScripts map without the p2sh script
also having to be added to the mapScripts map. This restores behavior prior to
https://github.com/bitcoin/bitcoin/pull/17261, which I think broke backwards
compatibility with old wallet files by no longer treating addresses created by
`addmultisigaddress` calls before #17261 as solvable.
The reason why tests didn't fail with the CanProvide implementation in #17261
is because of a workaround added in 4a7e43e8460127a40a7895519587399feff3b682
"Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem
for new `addmultisigaddress` RPC calls without fixing it for multisig addresses
already created in old wallet files.
This change adds a lot of comments and allows reverting commit
4a7e43e8460127a40a7895519587399feff3b682 "Store p2sh scripts in
AddAndGetDestinationForScript", so the AddAndGetDestinationForScript() function,
CanProvide() method, and mapScripts map should all be more comprehensible
|
|
This makes the rendered diff smaller when the RPCResult is machine
generated later on
-BEGIN VERIFY SCRIPT-
# Add space after dictionary key and before colon
sed -i --regexp-extended -e 's/(^ +" +\\"[a-zA-Z_]+\\"): ?/\1 : /g' $(git grep -l '\\":')
# Rename (array) to (json array)
sed -i -e 's/ (array) / (json array) /g' $(git grep -l '(array)' ./src)
# Rename (object) to (json object)
sed -i -e 's/ (object) / (json object) /g' $(git grep -l '(object)' ./src)
# Rename (bool) to (boolean)
sed -i -e 's/ (bool) / (boolean) /g' $(git grep -l '(bool)' ./src)
# Rename (int) to (numeric)
sed -i -e 's/ (int) / (numeric) /g' $(git grep -l '(int)' ./src)
-END VERIFY SCRIPT-
|
|
19a354b11f85a3c6c81ff83bf702bf7a40cf5046 Output a descriptor in createmultisig and addmultisigaddress (Andrew Chow)
Pull request description:
Give a descriptor from `createmultisig` and `addmultisigaddress`.
Extracted from #16528 with `addmultisgaddress` and tests added.
ACKs for top commit:
Sjors:
tACK 19a354b11f85a3c6c81ff83bf702bf7a40cf5046
MarcoFalke:
ACK 19a354b11f85a3c6c81ff83bf702bf7a40cf5046
promag:
Code review ACK 19a354b11f85a3c6c81ff83bf702bf7a40cf5046.
meshcollider:
utACK 19a354b11f85a3c6c81ff83bf702bf7a40cf5046
Tree-SHA512: e813125fbbc358ea8d45b1748de16a29a94efd83175b748fb8fa3b0bfc8e783ed36b6c554d84f5d4ead1ba252a83a3e937b6c3f75da7b8d3b4e55f94d6013771
|
|
fa5c6622c8ecf1954e7177888ad8c97a77b16fb7 doc: Use proper RPC help syntax in importmulti (MarcoFalke)
fab63111bec73859597e6ce0986f76e5e9959091 doc: Remove duplicate "comment" from listsinceblock RPC help (MarcoFalke)
fa04cd6cfc0330b62058ed169d621e08108dc87e doc: Properly document proxy_randomize_credentials as bool in getnetworkinfo (MarcoFalke)
fa9dec7c395897e8dbbb6de7a16ec5185a609d41 doc: Fix syntax error (trailing square bracket) in finalizepsbt (MarcoFalke)
faff5a60ed328d4c5fdef253e8935a351cb57bd0 doc: Fix syntax error (trailing square bracket) in walletprocesspsbt (MarcoFalke)
fa0545901daad32b09511cc61c4af1400c48088d doc: Add missing "optional" to "long" estimaterawfee RPC help (MarcoFalke)
Pull request description:
This fixes documentation of the following RPCs:
* estimaterawfee (hidden)
* https://bitcoincore.org/en/doc/0.19.0/rpc/wallet/walletprocesspsbt/
* https://bitcoincore.org/en/doc/0.19.0/rpc/rawtransactions/finalizepsbt/
* https://bitcoincore.org/en/doc/0.19.0/rpc/network/getnetworkinfo/
* https://bitcoincore.org/en/doc/0.19.0/rpc/wallet/listsinceblock/
* https://bitcoincore.org/en/doc/0.19.0/rpc/wallet/importmulti/
<!-- Also, it comes with a scripted diff to normalize whitespace and type names. (Previous attempts: #14601 and #14459)
ACKs for top commit:
laanwj:
ACK fa5c6622c8ecf1954e7177888ad8c97a77b16fb7
Tree-SHA512: 5a10956e12f8ce23e93a2ce8bafd6cae759d8a21658f79397e3bfce3e4aabd9658bdbd40acde49323dca958a9befee7166654994208c182dd60f483109621e17
|
|
e9434ee03efdfc0a5a54cf46561e95fd93cba007 Remove false positive GCC warning (Hennadii Stepanov)
Pull request description:
On master (f05c1ac444e0c893516535bfdf07c5c8cd9bce16) GCC compiler fires a false positive `-Wmaybe-uninitialized`:
```
wallet/wallet.cpp: In static member function āstatic std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain&, const WalletLocation&, std::__cxx11::string&, std::vector<std::__cxx11::basic_string<char> >&, uint64_t)ā:
wallet/wallet.cpp:3913:27: warning: ā*((void*)& time_first_key +8)ā may be used uninitialized in this function [-Wmaybe-uninitialized]
Optional<int64_t> time_first_key;
^~~~~~~~~~~~~~
```
The same as #15292.
This PR leverages a workaround and removes the warning.
ACKs for top commit:
laanwj:
ACK e9434ee03efdfc0a5a54cf46561e95fd93cba007, removes the warning for me (gcc 7.4.0)
kristapsk:
ACK e9434ee03efdfc0a5a54cf46561e95fd93cba007
Tree-SHA512: 8820a8ba6a75aa6b1ac675a38c883a77f12968b010533b6383180aa66e7e0d570bf6300744903ead91cf9084e5345144959cd6b0cea1b763190b8dd49bacce75
|
|
d3bc18408146e91b3836f72360ff6fa2420b6887 doc: update release notes with getaddressinfo label deprecation (Jon Atack)
72af93f36479dc12d795f1d05fa3d8fbd9b293bd test: getaddressinfo label deprecation test (Jon Atack)
d48875fa20d0b71b978cb3d1f85dd9ec14e664cc rpc: deprecate getaddressinfo label field (Jon Atack)
dc0cabeda49a7edbfa71df22846721b6f6224aea test: remove getaddressinfo label tests (Jon Atack)
c7654af6f830577a54df12b5d65df93532db0dc2 doc: address pr17578 review feedback (Jon Atack)
Pull request description:
This PR builds on #17578 (now merged) and deprecates the rpc getaddressinfo `label` field. The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=label`.
See http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622 and https://github.com/bitcoin/bitcoin/pull/17283#issuecomment-554458001 for more context.
Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=label` flag while verifying the rpc getaddressinfo output and help text.
Next step: add support for multiple labels.
ACKs for top commit:
jnewbery:
ACK d3bc18408146e91b3836f72360ff6fa2420b6887
laanwj:
ACK d3bc18408146e91b3836f72360ff6fa2420b6887
meshcollider:
utACK d3bc18408146e91b3836f72360ff6fa2420b6887
Tree-SHA512: f954402884ec54977def332c8160fd892f289b0d2aee1e91fed9ac3220f7e5b1f7fc6421b84cc7a5c824a0582eca4e6fc194e4e33ddd378c733c8941ac45f56d
|
|
|
|
|
|
|
|
have multiple
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 https://github.com/bitcoin/bitcoin/pull/17261/commits/3f373659d732a5b1e5fdc692a45b2b8179f66bec
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
|
|
f41d58966995fe69df433fa684117fae74a56e66 Document better -keypool as a look-ahead safety mechanism (Antoine Riard)
Pull request description:
If after a backup, an address is issued beyond the initial
keypool range and none of the addresses in this range
is seen onchain, if a wallet is restored from backup, even in
case of rescan, funds may be loss due to the look-ahead
buffer not being incremented and so restored wallet not detecting
onchain out-of-range address as derived from its seed.
This scenario is theoretically unavoidable due to the requirement
of the keypool to have a max size. However, given the default
keypool size, this is unlikely. Document better keypool size
implications to avoid user setting a too low value.
While reviewing #17681, it took me a while to figure out the safety implications of keypool, I find it would be better to document this a bit farther to avoid users shooting themselves in the foot. For further context & discussion, see https://github.com/bitcoin/bitcoin/pull/17681#issuecomment-563613452
ACKs for top commit:
ryanofsky:
Code review ACK f41d58966995fe69df433fa684117fae74a56e66. Just "Warning:" prefix added since the last review
jonatack:
ACK f41d58966995fe69df433fa684117fae74a56e66 code review and build/test. The added `Warning:` since last review is a good addition.
Tree-SHA512: d3d0ee88fcdfc5c8841a2bd4bada0e4eeb412a0dce5054e5fb023643c2fa57206a0f3efb06890c245528dc4431413ed2fd5645b9319d26245d044c490b7f0db0
|
|
deaa6dd144f5650b385658a0c4f9a014aff8dde2 psbt: check output index is within bounds before accessing (Andrew Chow)
f1ef7f0aa46338f4cd8de79696027a1bf868f359 Don't calculate tx fees for PSBTs with invalid money values (Andrew Chow)
Pull request description:
Fixes #17149
Two classes of issues were found by the psbt fuzzer: values out of range and causing overflows, and prevout indexes being out of range. This PR fixes both.
When accessing a specific output using the index given in the tx, check that it is actually a possible output before trying to access the output.
When summing and checking amounts for `decodepsbt` and `analyzepsbt`, make sure that the values are actually valid money values.. Otherwise, stop summing and don't show the fee. For `analyzepsbt`, return that the next role is the Creator since the Creator needs to remake the transaction to be valid.
ACKs for top commit:
practicalswift:
ACK deaa6dd144f5650b385658a0c4f9a014aff8dde2 -- only change since last ACK was the addition of tests
gwillen:
tested ACK deaa6dd, would also like to see this merged!
Tree-SHA512: 06c36720bbb5a7ab1c29f7d15878bf9f0d3e5760c06bff479d412e1bf07bb3e0e9ab6cca820a4bfedaab71bfd7af813807e87cbcdf0af25cc3f66a53a06dbcfd
|
|
Needed for future ScriptPubKeyMans which may need to create
SigningProviders dynamically and thus a normal pointer is not enough
This commit does not change behavior.
|
|
This commit does not change behavior.
|
|
Add wallet logic for dealing with multiple ScriptPubKeyMan instances. This
doesn't change current behavior because there is still only a single
LegacyScriptPubKeyMan. But in the future the new logic will be used to support
descriptor wallets.
|
|
Instead of having a uint256 representations of one scattered throughout
where it is used, define it globally in uint256.h
|
|
This avoids repeaded upgrades when support for more multiple keyman references
is added in the next commit:
https://github.com/bitcoin/bitcoin/pull/16341#discussion_r322370108
|
|
|
|
In CWallet::LoadWallet, use this to detect and empty wallet with no keys
This commit does not change behavior.
|
|
This commit only affects locking behavior and doesn't have other changes.
|
|
|
|
|
|
Also, properly document all (json object) and (json array)
|
|
|
|
297e09855793feb94c3229ed989bef8b1eac864e Fix doxygen errors (Ben Woosley)
Pull request description:
These are all the remaining errors identified via -Werror=documentation, e.g.:
```
./rpc/rawtransaction_util.h:31:13: error: parameter 'prevTxs' not found in the function declaration [-Werror,-Wdocumentation]
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
./rpc/rawtransaction_util.h:31:13: note: did you mean 'prevTxsUnival'?
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
prevTxsUnival
netbase.cpp:766:11: error: parameter 'outProxyConnectionFailed[out]' not found in the function declaration [-Werror,-Wdocumentation]
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
netbase.cpp:766:11: note: did you mean 'outProxyConnectionFailed'?
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
outProxyConnectionFailed
```
You can use this to run with `-Wdocumentation` yourself: #14920
ACKs for top commit:
laanwj:
ACK 297e09855793feb94c3229ed989bef8b1eac864e
Tree-SHA512: a232d893b170873d923e77fa56c56a6567e7fd120b5af1f52cfeeae1093eec55621604cc80a523678f6fedc8bbb31228c4aa8dc2a630ce9ffc91525988522af7
|
|
Identified via -Wdocumentation, e.g.:
./rpc/rawtransaction_util.h:31:13: error: parameter 'prevTxs' not found in the function declaration [-Werror,-Wdocumentation]
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
./rpc/rawtransaction_util.h:31:13: note: did you mean 'prevTxsUnival'?
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
prevTxsUnival
netbase.cpp:766:11: error: parameter 'outProxyConnectionFailed[out]' not found in the function declaration [-Werror,-Wdocumentation]
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
netbase.cpp:766:11: note: did you mean 'outProxyConnectionFailed'?
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
outProxyConnectionFailed
|
|
2b1641492fbf81e2c5a95f3e580811ca8700adc5 wallet: Improve CWallet:MarkDestinationsDirty (JoĆ£o Barbosa)
Pull request description:
Improve `CWallet:MarkDestinationsDirty` by skipping transactions that already have the cache invalidated. Skipping a transaction avoids at worst case extracting all output destinations.
ACKs for top commit:
meshcollider:
re-utACK 2b1641492fbf81e2c5a95f3e580811ca8700adc5
Tree-SHA512: 479dc2dde4b653b856e3d6a0c59a34fe33e963eb131a2d88552a8b30471b8725a087888fe5d7db6e4ee19b74072fe64441497f033be7d1931637f756e0d8fef5
|
|
|
|
ScriptHash
6dd59d2e491bc11ab26498668543e65440a3a931 Don't allow implementers to think ScriptHash(Witness*()) results in nesting computation (Gregory Sanders)
4b8f1e989f3b969dc628b0801d5c31ebd373719c IsUsedDestination shouldn't use key id as script id for ScriptHash (Gregory Sanders)
Pull request description:
Regression introduced in https://github.com/bitcoin/bitcoin/pull/17621 which causes p2sh-segwit addresses to be erroneously missed.
Tests are only failing in 0.19 branch, likely because that release still uses p2sh-segwit addresses rather than bech32 by default.
I'll devise a test case to catch this going forward.
ACKs for top commit:
achow101:
ACK 6dd59d2e491bc11ab26498668543e65440a3a931
MarcoFalke:
ACK 6dd59d2
meshcollider:
Code review ACK 6dd59d2e491bc11ab26498668543e65440a3a931
Tree-SHA512: b3e0f320c97b8c1f814cc386840240cbde2761fee9711617b713d3f75a4a5dce2dff2df573d80873df42a1f4b74e816ab8552a573fa1d62c344997fbb6af9950
|