Age | Commit message (Collapse) | Author |
|
|
|
9e95931865186d7a9a6dc54b64bd96507e9fea4b [wallet] Remove `state` argument from CWallet::CommitTransaction (John Newbery)
d1734f9a3b138ab046f38ee44a09bc3847bf938a [wallet] Remove return value from CommitTransaction() (John Newbery)
b6f486a02b463ffeaf82ec11fc6f74f439c037ae [wallet] Add doxygen comment to CWallet::CommitTransaction() (John Newbery)
8bba91b22d22a8dfea7c947b542b1022bfc1c0ea [wallet] Fix whitespace in CWallet::CommitTransaction() (John Newbery)
Pull request description:
`CommitTransaction()` returns a bool to indicate success, but since commit
b3a7410 (#9302) it only returns true, even if the transaction was not
successfully broadcast. This commit changes CommitTransaction() to return
void.
All dead code in `if (!CommitTransaction())` branches has been removed.
Two additional commits fix up the idiosyncratic whitespace in `CommitTransaction` and add a doxygen comment for the function.
ACKs for top commit:
laanwj:
ACK 9e95931865186d7a9a6dc54b64bd96507e9fea4b
Tree-SHA512: a55a2c20369a45222fc0e02d0891495655a926e71c4f52cb72624768dd7b9c1dca716ea67d38420afb90f40c6e0fd448caa60c18fd693bb10ecb110b641820e6
|
|
cd68594dcdadc195bd2ea9394fa04edfdbdf1149 Only check the hash of transactions loaded from disk (Andrew Chow)
Pull request description:
It feels unnecessary to do a full `CheckTransaction` for every transaction saved in the wallet. It should not be possible for an invalid transaction to get into the wallet in the first place, and if there is any disk corruption, the hash check will catch it.
ACKs for top commit:
MarcoFalke:
ACK cd68594dcdadc195bd2ea9394fa04edfdbdf1149
laanwj:
ACK cd68594dcdadc195bd2ea9394fa04edfdbdf1149
promag:
ACK cd68594dcdadc195bd2ea9394fa04edfdbdf1149, AFAICT the check is not needed, hash comparison gives data integrity.
Tree-SHA512: 5b2e719f76097cfbf125392db6cc6c764355c81f0b7a5b60aee4b06af1afcca80cfd38a3cf5307fd9e2c1afc405f8321929a4552943099a8161e6762965451fb
|
|
facec1c643105d0ae74b5d32cf33d593f9e82a36 wallet: Avoid showing GUI popups on RPC errors (MarcoFalke)
Pull request description:
RPC errors and warnings are shown as popups in the GUI instead of being returned to the RPC caller. For example,
```
$ ./src/bitcoin-cli loadwallet $(pwd)/./test/functional/data/wallets/high_minversion/
error code: -4
error message:
Wallet loading failed.
```
gives me a GUI popup and no reason why loading the wallet failed.
After this pull request:
```
$ ./src/bitcoin-cli loadwallet $(pwd)/./test/functional/data/wallets/high_minversion/
error code: -4
error message:
Wallet loading failed: Error loading /home/marco/workspace/btc_bitcoin_core/./test/functional/data/wallets/high_minversion/wallet.dat: Wallet requires newer version of Bitcoin Core
ACKs for top commit:
laanwj:
Code review ACK facec1c643105d0ae74b5d32cf33d593f9e82a36
Tree-SHA512: c8274bbb02cfcf71676eeec1e773e51fb3538cf93f82e7cb8536f4716d44ed819cdc162dfc039ac7386a4db381a734cdb27fd32567043a1180c02519fbcba194
|
|
|
|
The `state` return argument has not been set since commit 611291c198.
Remove it (and the one place that it's used in a calling function).
|
|
CommitTransaction returns a bool to indicate success, but since commit
b3a74100b8 it only returns true, even if the transaction was not
successfully broadcast. This commit changes CommitTransaction() to return
void.
All dead code in `if (!CommitTransaction())` branches has been removed.
|
|
|
|
Reviewer hint: use --ignore-all-space git diff option for review.
|
|
|
|
b96ed0396294fc4fa89d83ceab6bc169dd09f002 [wallet] Remove pruning check for -rescan option (John Newbery)
eea462de9c652dca556ad241d2126b10790f67f8 [wallet] Remove package limit config access from wallet (John Newbery)
Pull request description:
Removes wallet access to `-limitancestorcount`, `-limitdescendantcount` and `-prune`:
- `-limitancestorcount` and `-limitdescendantcount` are now accessed with a method `getPackageLimits` in the `Chain` interface.
- `-prune` is not required. It was only used in wallet component initiation to prevent running `-rescan` when pruning was enabled. This check is not required.
Partially addresses #17137.
ACKs for top commit:
MarcoFalke:
Tested ACK b96ed0396294fc4fa89d83ceab6bc169dd09f002
ryanofsky:
Code review ACK b96ed0396294fc4fa89d83ceab6bc169dd09f002
promag:
Code review ACK b96ed0396294fc4fa89d83ceab6bc169dd09f002.
ariard:
ACK b96ed03, check there isn't left anymore wallet access to node arguments.
Tree-SHA512: 90c8e3e083acbd37724f1bccf63dab642cf9ae95cc5e684872a67443ae048b4fdbf57b52ea47c5a1da6489fd277278fe2d9bbe95e17f3d4965a1a0fbdeb815bf
|
|
66b29848c71c4b3b4dc36ca6d94de829bd533797 change wallet pointers to references in feebumper (Adam Jonas)
9be6666a4e648782b49a6fdc458372ea521b444d typo and unneccessary parentheses (Adam Jonas)
Pull request description:
Picking up some of the suggestions in the comments of #16727 including:
https://github.com/bitcoin/bitcoin/pull/16727#discussion_r330547321
https://github.com/bitcoin/bitcoin/pull/16727#discussion_r330549766
https://github.com/bitcoin/bitcoin/pull/16727#discussion_r333209674
ACKs for top commit:
promag:
Code review ACK 66b29848c71c4b3b4dc36ca6d94de829bd533797.
MarcoFalke:
ACK 66b29848c71c4b3b4dc36ca6d94de829bd533797 (looked at the diff on GitHub)
fjahr:
ACK 66b2984 reviewed code
Tree-SHA512: d118f7689970fe39d9f5318dc818f13283cce9194370b3ce4758f298172e4681ae119ddc809f5c0b7602677137ac0d38147b915422ff616531a76a570b766fa2
|
|
Prior to this PR, the wallet would not allow the `-rescan` option at
startup if pruning was enabled. This is unnecessarily restrictive. It
should be possible to rescan if pruning is enabled, as long as no blocks
have actually been pruned yet.
Remove the pruning check from WalletInit::ParameterInteraction(). If any
blocks have been pruned, that will be caught in CreateWalletFromFile().
|
|
The wallet should not be able to directly access global configuration
from the node. Remove access of "-limitancestorcount" and
"-limitdescendantcount".
|
|
|
|
PubKeys
a57a1d42d52fe51e5b413a1fd3a5ef2b7a2120e3 test: add unit test for wallet watch-only methods involving PubKeys (Sebastian Falbesoner)
Pull request description:
The motivation for this addition was to unit test the function `wallet.cpp:ExtractPubKey()` (see recent change in commit 798a589aff64b83a0844688a661f4bd987c3340c) which is however static and only indirectly available via the public methods `AddWatchOnly()`, `LoadWatchOnly()` and `RemoveWatchOnly()`. Since the first of those methods also stores the addresses to the disk, the second, simpler one was chosen which only operates in memory.
ACKs for top commit:
Sjors:
ACK a57a1d4
instagibbs:
reACK https://github.com/bitcoin/bitcoin/pull/16786/commits/a57a1d42d52fe51e5b413a1fd3a5ef2b7a2120e3
Sjors:
re-ACK a57a1d4
Tree-SHA512: 92a242204ab533022cd848662997372c41815b1265d07b3d96305697f801db29a5ba5668337faf4bea702bec1451972529afd6665927fb142aaf91700a338b26
|
|
|
|
Fields involvesWatchonly, generated, walletconflicts were missing
in result description of listtransactions, listsinceblock,
gettransaction
Align getttransaction fields which were odd compare to other rpc
helpers
|
|
|
|
|
|
|
|
ea4cc3a7b36a9c77dbf0aff439da3ef0ea58e6e4 Truly decouple wallet from chainparams for -fallbackfee (Jorge Timón)
Pull request description:
Before it was 0 by default for main and 20000 for test and regtest.
Now it is 0 by default for all chains, thus there's no need to call Params().
Also now the default for main is properly documented.
Suggestion for release notes:
-fallbackfee was 0 (disabled) by default for the main chain, but 20000 by default for the test chains. Now it is 0 by default for all chains. Testnet and regtest users will have to add fallbackfee=20000 to their configuration if they weren't setting it and they want it to keep working like before.
Should I propose them to the wiki for the release notes or only after merge?
For more context, see https://github.com/bitcoin/bitcoin/pull/16402#issuecomment-515701042
ACKs for top commit:
MarcoFalke:
ACK ea4cc3a7b36a9c77dbf0aff439da3ef0ea58e6e4
Tree-SHA512: fdfaba5d813da4221e405e0988bef44f3856d10f897a94f9614386d14b7716f4326ab8a6646e26d41ef3f4fa61b936191e216b1b605e9ab0520b0657fc162e6c
|
|
Before it was 0 by default for main and 20000 for test and regtest.
Now it is 0 by default for all chains, thus there's no need to call Params().
Also now the default for main is properly documented
|
|
71d4eddf42eb5a15e434d2273f11d0a3942ef502 Add release note for bech32 by default in wallet (Gregory Sanders)
b34f0180e39fc95d88596a987724b994707e2552 Revert "gui: Generate bech32 addresses by default (take 2, fixup)" (Gregory Sanders)
f50785ab56c0c094960c7049cfe9209b101e2823 Change default address type to bech32 (Gregory Sanders)
Pull request description:
ACKs for top commit:
MarcoFalke:
re-ACK 71d4eddf42eb5a15e434d2273f11d0a3942ef502 (only change is restore mimick behavior)
laanwj:
ACK 71d4eddf42eb5a15e434d2273f11d0a3942ef502
Tree-SHA512: 3c49a1b51c49f3a762ad08985167ca1b89b0177ae20ab6d5883f1f74dde7a155921c1b855a842199bbf32f563c56b33f8b603bc842637bdcb121001023d454b6
|
|
c812aba3949b6ab81030dc708cda7c8821be2f70 test bumpfee fee_rate argument (ezegom)
9f25de3d9eb8d012ca1a98cbcd28021e3e1c85ee rpc bumpfee check fee_rate argument (ezegom)
88e5f997dfab3f03bb1ec3f149eaff8dcc2981fe rpc bumpfee: add fee_rate argument (ezegom)
1a4c791cf49ff15aa9deba4388c0180b8f47f15b rpc bumpfee: move feerate estimation logic into separate method (ezegom)
Pull request description:
Taking over for https://github.com/bitcoin/bitcoin/pull/16492 which seems to have gone inactive.
Only minor commit cleanups, rebase, and some help text fixes on top of previous PR. Renamed `feeRate` to `fee_rate` to reflect updated guidelines.
ACKs for top commit:
Sjors:
Code review ACK c812aba
laanwj:
ACK c812aba3949b6ab81030dc708cda7c8821be2f70
Tree-SHA512: 5f7f51bd780a573ccef1ccd72b0faf3e5d143f6551060a667560c5163f7d9480e17e73775d1d7bcac0463f3b6b4328f0cff7b27e39483bddc42a530f4583ce30
|
|
addaf8af8268d918973883a304025d40af5a33c1 make sure to update the UI when deleting a transaction (Jonas Schnelli)
Pull request description:
`CWallet::ZapSelectTx` removes transactions from the internal model, but leaves the UI in the dark.
Adding a `NotifyTransactionChanged()` should avoid having invalid transactions in the GUI.
Fixes #16950
ACKs for top commit:
fanquake:
ACK addaf8af8268d918973883a304025d40af5a33c1 - tested that this fixes #16950
Sjors:
tACK addaf8a: tested with an unpruned wallet by calling `removeprunedfunds` on an RBF-replaced transaction. It neatly disappears from the UI.
kristapsk:
ACK addaf8af8268d918973883a304025d40af5a33c1 (tested both with and without this change)
Tree-SHA512: 65e8c690847f7499e82c9fef67b60d9aaa63c853732fe7fa7281da33054fcdcd9d24f5b86de71b0827728c25bac8efb7db445863f990304ebfee6fc450620c47
|
|
80031045fc6435ef4eb5dd1aee773d938c57a0fd Clarify includeWatching for fundrawtransaction (Steven Roose)
Pull request description:
Might be sufficient to solve https://github.com/bitcoin/bitcoin/issues/16396, https://github.com/bitcoin/bitcoin/issues/7879 and https://github.com/bitcoin/bitcoin/issues/14405.
ACKs for top commit:
Sjors:
ACK 8003104. This will always be confusing, but at least it gives a bunch more clues for the user to google.
Tree-SHA512: 9b8002c259c50f93d89fc5574105aae6152858d8d45c07b4c3d5b7023adafe73c7a98a290874ff3fbbb7dfad2ac1bdf4acb8769a2a1c14e38484922f44e84e54
|
|
|
|
|
|
|
|
|
|
|
|
The motivation for this addition was to unit test the function
wallet.cpp:ExtractPubKey() (see recent change in commit
798a589aff64b83a0844688a661f4bd987c3340c) which is however static and only
indirectly available via the public methods AddWatchOnly(), LoadWatchOnly() and
RemoveWatchOnly(). Since the first of those methods also stores the addresses
to the disk, the second, simpler one was chosen which only operates in memory.
test: add missing wallet lock for test case WatchOnlyPubKeys
test: test case WatchOnlyPubKeys, suggested review changes by instagibbs
test: test case WatchOnlyPubKeys, suggested review changes by achow101
test: test case WatchOnlyPubKeys, s/isPubKeyFullyValid/is_pubkey_fully_valid
|
|
PR 16866 renamed the 'decode' argument in gettransaction to 'verbose' to make it more consistent with other RPC calls like getrawtransaction.
However, it seems it inadvertently overloaded the 'details' fields when 'verbose' is passed. The result is that the original 'details' fields are no longer returned, which seems to be a breaking API change.
This PR takes the simplest path to restoring the 'details' fields by renaming them from 'details' back to 'decoded', while leaving the 'verbose' argument for API consistency.
It also addresses [this comment](https://github.com/bitcoin/bitcoin/pull/16185#discussion_r320740413) to mention that the 'decoded' field is identical to decoderawtransaction.
Update the RPC help, functional test, and release note.
|
|
This makes the RPC method consistent with other RPC methods that have a
'verbose' option.
Change the name of the return object from 'decoded' to details.
Update help text.
|
|
|
|
fa734603b78ba31ebf0da5d2dbe87386eafff01a wallet: Fix segmentation fault in CreateWalletFromFile (MarcoFalke)
fab3c34412379598b812631e3c123e9467cdc485 test: Print both messages on failure in assert_raises_message (MarcoFalke)
faa13539d5262bb7a512e9ff82e80083e04315ee wallet: Fix documentation around WalletParameterInteraction (MarcoFalke)
Pull request description:
Comes with a test to aid review. The test should fail without the fix to bitcoind
The following `CreateWalletFromFile` issues are fixed:
* `walletFile` refers to freed memory and will thus corrupt the debug.log and/or crash the node if read
* `WalletParameterInteraction` was moved to `CreateWalletFromFile` and `WalletInit::ParameterInteraction` without updating the documentation
ACKs for top commit:
promag:
ACK fa734603b78ba31ebf0da5d2dbe87386eafff01a.
darosior:
ACK fa734603b78ba31ebf0da5d2dbe87386eafff01a
meshcollider:
LGTM, code-read ACK fa734603b78ba31ebf0da5d2dbe87386eafff01a
Tree-SHA512: 2aceb63a3f25b90a840cfa08d37f5874aad4eb3df8c2ebf94e2ed18b55809b185e6920bdb345b988bff1fcea5e68a214fe06c361f7da2c01a3cc29e0cc421cb4
|
|
prevtx parsing
39034f1ee628dae0bc9da5b1b30b8a424e66d968 Refactor rawtransaction_util's SignTransaction to have previous tx parsing be separate (Andrew Chow)
Pull request description:
Currently the `SignTransaction` function has to handle both the actual signing and parsing of previous transaction data. This PR splits it so that `SignTransaction` only handles the signing itself and adds a `ParsePrevouts` function which handles parsing the prevtx information.
This allows for `SignTransaction` to just take any `SigningProvider`.
Split from #16341
ACKs for top commit:
MarcoFalke:
ACK 39034f1ee628dae0bc9da5b1b30b8a424e66d968
instagibbs:
utACK https://github.com/bitcoin/bitcoin/pull/16798/commits/39034f1ee628dae0bc9da5b1b30b8a424e66d968
ryanofsky:
utACK 39034f1ee628dae0bc9da5b1b30b8a424e66d968. No change since previously reviewed b49bbb939be92a67ff77c3f7bca5bb94dd141906, https://github.com/bitcoin/bitcoin/pull/16341#pullrequestreview-278610269 other than rebase with no conflicts.
Tree-SHA512: 09f7733e90691766bfb5cf0f20e913dbf270bd3b51abdcad966b24d110e562ed85fd3d0d1d7bbea61f903340060052ec73c4817b09aee0dc1f3916d781a9e40c
|
|
442a87cc0ae43ebc9b6654a6165778eecb931f74 Add a test wallet_reorgsrestore (Antoine Riard)
40ede992d97df38282919693dfe851c975c3b1d8 Modify wallet tx status if has been reorged out (Antoine Riard)
7e89994133725125dddbfa8d45484e3b9ed51c6e Remove SyncTransaction for conflicted txn in CWallet::BlockConnected (Antoine Riard)
a31be09bfd77eed497a8e251d31358e16e2f2eb1 Encapsulate tx status in a Confirmation struct (Antoine Riard)
Pull request description:
While working on #15931, I've tried to rationalize tx state management to ease integration of block height tracking per-wallet tx. We currently rely on a combination of `hashBlock` and `nIndex` with magic value to determine tx confirmation, conflicted or abandoned state. It's hard to reason and error-prone. To solve that, we encapsulate these fields in a `TxConfirmation` struct and introduce a `TxState` member that we update accordingly at block connection/disconnection.
Following jnewbery [recommendation](https://github.com/bitcoin/bitcoin/pull/15931#discussion_r312576506), I've taken these changes in its own commit, and open a PR to get them first. It would ease review of aforementioned PR, but above all should ease fixing of long-term issues like :
* https://github.com/bitcoin/bitcoin/issues/7315 (but maybe we should abandon abandontransaction or relieve it to only free outpoints not track the transaction as abandoned in itself, need its own discussion)
* https://github.com/bitcoin/bitcoin/issues/8692 where we should cancel conflicted state of transactions chain smoothly
* `MarkConflicted` in `LoadToWallet` is likely useless if we track conflicts rights at block connection
Main changes of this PR to get right are tx update in `AddToWallet` and serialization/deserialization logic.
ACKs for top commit:
meshcollider:
Light re-Code Review ACK 442a87cc0ae43ebc9b6654a6165778eecb931f74
ryanofsky:
utACK 442a87cc0ae43ebc9b6654a6165778eecb931f74. Changes since last review are switching from `hasChain` to `LockChain` and removing chain lock in `WalletBatch::LoadWallet` that's redundant with the new lock still added in `CWallet::LoadWallet`, and fixing python test race condition.
Tree-SHA512: 029209e006de0240436817204e69e548c5665e2b0721b214510e7aba7eba130a5eab441d3a1ad95bd6426114dd27390492c77bf4560a9610009b32cd0a1f72f7
|
|
be separate
|
|
|
|
|
|
fa61365a1368f9970fa22fb96f89f4ecc08e69f0 wallet: Translate all initErrors in CreateWalletFromFile (MarcoFalke)
fa70d199d0c2182d76b2a1cfa21f9ada4bb12913 util: Make util/error bilingual_str (refactor) (MarcoFalke)
Pull request description:
The translations are going to close in three days (#15940), so I am submitting this as a standalone pull request.
Those changes are part of a bugfix #16661, which includes a test. The first change (the refactor) is required, the second commit is not. I am happy to drop it, if needed.
ACKs for top commit:
laanwj:
utACK fa61365a1368f9970fa22fb96f89f4ecc08e69f0
hebasto:
ACK fa61365a1368f9970fa22fb96f89f4ecc08e69f0, I have tested the code on Linux Mint 19.2.
Tree-SHA512: a7616cc38b9ffd301c6b915ea808a65815c3d97e9f57ec091772eb260e5cf0d75a13a6e4dfa3913e236833677c7929b9a748cb7d7a0e406d51749944b614e11b
|
|
9965940e35c445ccded55510348af228ff22f0e9 doc: Add release note for the new gettransaction argument (darosior)
b8b3f0435a2837d3897e9e232ef6ca839ce74eb8 tests: Add a new functional test for gettransaction (darosior)
7f3bb247a811582d1aa4805d8e601c19808dc7ba gettransaction: add an argument to decode the transaction (darosior)
Pull request description:
This PR adds a new parameter to the `gettransaction` call : `decode`. If set to `true`, it will add a new `decoded` field to the response. This mimics the behavior of `getrawtransaction`'s `verbose` argument to avoid using 2 calls if we want to decode a wallet transaction (`gettransaction` then `decoderawtransaction`).
Fix #16181 .
ACKs for top commit:
meshcollider:
re-utACK 9965940e35c445ccded55510348af228ff22f0e9
Tree-SHA512: bcb6b4bd252b3488d6afc77659c499c2ad99fd58661eb24b6a2e17014c74f22e47fde70e00fedb4f4754915786622ad02483b2cf2c4dea0ab0eb4ac8276dbeee
|
|
d9d8984270dbb004ec94f8dbb289be2bc9e4dbc3 wallet: Use wallet name instead of pointer on unload/release (João Barbosa)
Pull request description:
Fixes #16668. Wallet name is unique so it can be used instead of pointer.
ACKs for top commit:
meshcollider:
utACK d9d8984270dbb004ec94f8dbb289be2bc9e4dbc3
instagibbs:
utACK https://github.com/bitcoin/bitcoin/pull/16716/commits/d9d8984270dbb004ec94f8dbb289be2bc9e4dbc3
ryanofsky:
utACK d9d8984270dbb004ec94f8dbb289be2bc9e4dbc3. Alternately I think it might be possible to use an intptr_t set instead of a string set to get around the undefined behavior described in the issue.
Tree-SHA512: eccd4d260cd4c02b52c30deeb32dbfd190a1151a5340eb3aa4ece0dc6ae3b3ed746ce5617336461f6f27c437c435629cd07d20beb1c5450f23b75edde6728598
|
|
This adds a new boolean parameter 'decode' to the gettransaction call, which, if set to true, add a 'decoded' field to the result containing the decoded transaction
|
|
Add a LockChain method to CWallet to know if we can lock or query
chain state safely.
At tx loading, we rely on chain to know if hashBlock of tx is still
in main chain. If not, we set its status to unconfirmed and reset
its hashBlock/nIndex.
If wallet loaded is the wallet-tool one, all wallet txn will
show up with a height of zero. It doesn't matter as status is not
used by wallet-tool.
We take lock prematurely in CWallet::LoadWallet and CWallet::Verify
to ensure that lock order is respected between cs_main an cs_wallet.
|
|
The function ExtractPubKey() checks if a given script matches the P2PK pattern
(<PubKey> OP_CHECKSIG), extracts the PubKey and additionally checks if it is
cryptographically valid (full validation with ECC library via .IsFullyValid()).
Currently this is done manually in the following order:
1) check if first script OP is data push with valid PubKey length
(first part of pattern match), extract PubKey
2) create CPubKey object with extracted PubKey
3) fully validate public key
4) check if last script OP is OP_CHECKSIG
(second part of pattern match)
Using Solver, the pattern matching and PubKey extraction can be done via a
single step, leading to the following simplified order with shorter code:
1) check if given script matches P2PK pattern with Solver
(also contains valid PubKey length check), extracts Pubkey
2) create CPubKey object with extracted Pubkey
3) fully validate public key
|
|
Every warning or error in this method is translated, except for those
two. Translate them as well for consistency.
|
|
Translated strings should not end up in the debug log, stderr, or
returned by an RPC.
Changing the util methods in util/error to return a bilingual_str paves
the way to achieve this goal in the long term.
|