07e01d6258831fd2dfef959a405b3dfe4e88c3c1 rpc: sendrawtransaction unconditionality/privacy note (Jon Atack)
Pull request description:
In sendrawtransaction RPCHelpMan, mention unconditionality and privacy as per http://www.erisian.com.au/bitcoin-core-dev/log-2019-07-25.html#l-522
before
```
$ bitcoin-cli help sendrawtransaction
sendrawtransaction "hexstring" ( maxfeerate )
Submits raw transaction (serialized, hex-encoded) to local node and network.
Also see createrawtransaction and signrawtransactionwithkey calls.
(...)
```
after
```
$ bitcoin-cli help sendrawtransaction
sendrawtransaction "hexstring" ( maxfeerate )
Submit a raw transaction (serialized, hex-encoded) to local node and network.
Note that the transaction will be sent unconditionally to all peers, so using this
for manual rebroadcast may degrade privacy by leaking the transaction's origin, as
nodes will normally not rebroadcast non-wallet transactions already in their mempool.
Also see createrawtransaction and signrawtransactionwithkey calls.
(...)
```
ACKs for top commit:
promag:
ACK 07e01d6258831fd2dfef959a405b3dfe4e88c3c1.
laanwj:
ACK 07e01d6258831fd2dfef959a405b3dfe4e88c3c1
Tree-SHA512: 427b3ca29384eef271eb496b7b14e883220863543a536ddeb31940aaffd52ea0b607d929d50f2b7958514105ef7823fa05c1ee381d4a432808753c06bd97af58
35e60e790f2cd602d1bdd0be835d27f0ba37efa9 Remove ReadVersion and WriteVersion (Andrew Chow)
b3d4f6c9619142948ab3d53551b4f3c0d7d73bde Log the actual wallet file version (Andrew Chow)
c88e87c3b2be3f97b712107e04285d06dfef3878 Remove nFileVersion from CWalletScanState (Andrew Chow)
Pull request description:
The wallet file version is stored in the "minversion" record, not the "version" record. However "version" is no longer used anywhere except to record the highest versioned client which has opened a wallet file (which is currently only used to check whether this was most recently opened by a 0.4.0 or 0.5.0rc1 client which had a broken wallet encryption implementation). Furthermore, "version" was logged to the debug.log which is confusing because it is not the actual wallet file version.
This PR changes it so that this confusion largely no longer exists. The wallet file version logging is changed to use "minversion" and reading and writing the "version" record is no longer publicly exposed to prevent potential confusion about whether the actual file version is being read or written. Lastly, in the one place it is actually used, the variable name is changed from nFileVersion to last_client to better reflect what that record actually represents.
ACKs for top commit:
jb55:
ACK 35e60e7, I compiled locally as a quick sanity check.
ryanofsky:
utACK 35e60e790f2cd602d1bdd0be835d27f0ba37efa9. This code still pretty confusing, but a little simpler now. And the previous log statement was really misleading and useless compared to the new one here.
meshcollider:
Looks good, thanks! utACK 35e60e790f2cd602d1bdd0be835d27f0ba37efa9
Tree-SHA512: f782b2f215d07fbc9b806322bda8085445b81c02b65ca674a8c6a3e1de505a0abd050669afe0ead4778816144a1c18462e13930071cedb7227a058aeb39493f7
4d94916f0dda535cb69b538ee4e3fffb5b033c87 Get rid of PendingWalletTx class. (Russell Yanofsky)
Pull request description:
No reason for this class to exist if it doesn't have any code to run in the destructor. e10e1e8db043e9b7c113e07faf408f337c1b732d from https://github.com/bitcoin/bitcoin/pull/16208 recently removed the destructor code that would return an unused key if the transaction wasn't committed.
This is just cleanup, there's no change in behavior.
ACKs for top commit:
ariard:
utACK 4d94916. Successfully built both `bitcoind` and `bitcoin-qt`. `PendingWalletTx` was only a wrapper to enforce call to `ReturnDestination` if `CommitTransaction` doesn't `KeepDestination` before.
promag:
ACK 4d94916f0dda535cb69b538ee4e3fffb5b033c87, refactor looks good to me.
meshcollider:
utACK 4d94916f0dda535cb69b538ee4e3fffb5b033c87
Tree-SHA512: f3f93d2f2f5d8f1e7810d609d881c1b1cbbaa8629f483f4293e20b3210292605e947bc4903fde9d2d8736277ca3bd6de182f7eac1e13515d5a327f2ebc130839
fa4a605a4c611abe9af4c18aab20f4d1d039170f Remove wallet settings from chainparams (MarcoFalke)
Pull request description:
Feels a bit odd to have wallet setting in the chainparams, so remove them from there
ACKs for top commit:
promag:
ACK fa4a605a4c611abe9af4c18aab20f4d1d039170f, missed s/2018/2019?
practicalswift:
utACK fa4a605a4c611abe9af4c18aab20f4d1d039170f
darosior:
ACK fa4a605a4c611abe9af4c18aab20f4d1d039170f
Tree-SHA512: 2b3a5ee85d36af290d7db80bed1339e3c684607f1ce61cc65c906726e9174e40325fb1f67a34d8780f2a61fa39a1785e7c3a1cef5b6d6c364f38db5300cdbe3a
2f7eb772f6250442d4a0071318047cb2deeb31fa Add RPC bumpfee totalFee deprecation test (Jon Atack)
a92d9ce8cf355e18e43e1f207e4be9e42e7ec81a deprecate totalFee argument in bumpfee RPC call (Gregory Sanders)
Pull request description:
totalFee argument is of questionable use, and should be removed in favor of feerate-based features.
I first moved IsDeprecatedRPCEnabled because `bitcoin-wallet` doesn't link `libbitcoin_server`.
ACKs for top commit:
ryanofsky:
utACK 2f7eb772f6250442d4a0071318047cb2deeb31fa. Only change since last review is leaving IsDeprecatedRPCEnabled in its happy home, and switching to rpcEnableDeprecated instead. (Thanks!)
jonatack:
ACK 2f7eb772f6250442d4a0071318047cb2deeb31fa. Built locally, manually tested rpc bumpfee, help output ([gist](https://gist.github.com/jonatack/863673eacc02f9da39ff6d6712f9d837)), all tests pass. Travis failures appears to be unrelated, the [bitcoin builds are green](https://bitcoinbuilds.org/index.php?build=121).
meshcollider:
Code Review ACK 2f7eb772f6250442d4a0071318047cb2deeb31fa
Tree-SHA512: c97465205ee59575df37894bcbb6c4ecf8858dd8fe9d89503f9342b226768c1dcb553153bc9eb3055f7bf5eb41573e48b8efa57e083cd255793cbe5280f0026a
40ad2f6a58228c72c655e3061a19a63640419378 Have importwallet use ImportPrivKeys and ImportScripts (Andrew Chow)
78941da5baf6244c7c54e86cf8ce3e09ce60c239 Optionally allow ImportScripts to set script creation timestamp (Andrew Chow)
94bf156f391759420465b2ff8c44f5f150246c7f Have importaddress use ImportScripts and ImportScriptPubKeys (Andrew Chow)
a00d1e5ec5eb019f8bbeb060a2b09e341d360fe5 Have importpubkey use CWallet's ImportScriptPubKeys and ImportPubKeys functions (Andrew Chow)
c6a827424711333f6f66cf5f9d79e0e6884769de Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys (Andrew Chow)
fae7a5befd0b8746d84a6fde575e5b4ea46cb3c4 Log when an import is being skipped because we already have it (Andrew Chow)
ab28e31c9563bd2cd1e4a088ffd2479517dc83f2 Change ImportScriptPubKeys' internal to apply_label (Andrew Chow)
Pull request description:
#15741 introduced `ImportPrivKeys`, `ImportPubKeys`, `ImportScripts`, and `ImportScriptPubKeys` in `CWallet` which are used by `importmulti`. This PR changes the remaining `import*` RPCs (`importaddress`, `importprivkey`, `importpubkey`, and `importwallet`) to use these functions as well instead of directly adding the imported items to the wallet.
ACKs for top commit:
MarcoFalke:
ACK 40ad2f6a58228c72c655e3061a19a63640419378 (checked that behavior changes are mentioned in the commit body)
ryanofsky:
utACK 40ad2f6a58228c72c655e3061a19a63640419378. Only change since last review is a tweaked commit message (mentioning label update in importpubkey commit)
Sjors:
ACK 40ad2f6a5. Those extra tests also pass.
Tree-SHA512: 910e3bbe20b6f8809a47b7293775db234125615d886c7fd99c194f4cdf00c765eb1e24b1799260f1213b98c88f9bbe696796f36087c182925e567d44e9194c98
a47df13471e3168e2e02023fb20cdf2414141b36 [qa] Test disconnect block failure -> shutdown (Suhas Daftuar)
4433ed0f730cfd60eeba3694ff3c283ce2c0c8ee [validation] Crash if disconnecting a block fails (Suhas Daftuar)
Pull request description:
If we're unable to disconnect a block during normal operation, then that is a
failure of our local system (such as disk failure) or the chain that we are on
(eg CVE-2018-17144), but cannot be due to failure of the (more work) chain that
we're trying to validate.
We should abort rather than stay on a less work chain.
Fixes #14341.
ACKs for top commit:
practicalswift:
utACK a47df13471e3168e2e02023fb20cdf2414141b36
TheBlueMatt:
utACK a47df13471e3168e2e02023fb20cdf2414141b36. Didn't bother to review the test in detail, it looked fine. Debated whether invalidateblock should ever crash the node, but *not* crashing in the case of hitting a pruned block (which is the only change here) is clearly better, even if there are other cases I'd argue we should crash in.
ryanofsky:
utACK a47df13471e3168e2e02023fb20cdf2414141b36. Only change since last review is new comment.
promag:
ACK a47df1347, it takes awhile to quit (RPC connection timeouts) but that's unrelated - hope to fix that soon.
fanquake:
ACK a47df13471e3168e2e02023fb20cdf2414141b36
Tree-SHA512: 4dec8cef6e7dbbe513c138fc5821a7ceab855e603ece3c16185b51a3830ab7ebbc844a28827bf64e75326f45325991dcb672f13bd7baede53304f27289c4af8d
This prevents premature tries to access or create the default datadir.
This is useful when the -datadir option is specified and the default
datadir is unreachable.
This prevents premature tries to access or create the default datadir.
This is useful when the -datadir option is specified and the default
datadir is unreachable.
This prevents premature tries to access or create the default datadir.
This is useful when the -datadir option is specified and the default
datadir is unreachable.
Also removes the now unused ImportAddress and ImportScript from rpcdump.cpp
Behavior changes:
* No errors will be thrown when the script or key already exists in the wallet.
* If the key or script is already in the wallet, their labels will be updated.
-BEGIN VERIFY SCRIPT-
sed -i 's/inline std::string _(const char\* psz)/inline bilingual_str _(const char\* psz)/' src/util/translation.h
sed -i 's/return G_TRANSLATION_FUN ? (G_TRANSLATION_FUN)(psz) : psz;/return bilingual_str{psz, G_TRANSLATION_FUN ? (G_TRANSLATION_FUN)(psz) : psz};/' src/util/translation.h
sed -i 's/\b_("\([^"]\|\\"\)*")/&.translated/g' $(git grep --files-with-matches '\b_("' src)
echo Hard cases - multiline strings.
sed -i 's/"Visit %s for further information about the software.")/&.translated/g' src/init.cpp
sed -i "s/\"Only rebuild the block database if you are sure that your computer's date and time are correct\")/&.translated/g" src/init.cpp
sed -i 's/" restore from a backup.")/&.translated/g' src/wallet/db.cpp
sed -i 's/" or address book entries might be missing or incorrect.")/&.translated/g' src/wallet/wallet.cpp
echo Special case.
sed -i 's/_(COPYRIGHT_HOLDERS)/&.translated/' src/util/system.cpp test/lint/lint-format-strings.py
-END VERIFY SCRIPT-
fa6f402bde146f92ed131e0c9c8e15a55e723307 Call node->initError instead of InitError from GUI code (Russell Yanofsky)
fad2502240a1c440ef03ac3f880475702e418275 init: Use InitError for all errors in bitcoind/qt (MarcoFalke)
Pull request description:
Using the same InitError for startup error in the daemon and the gui makes it possible to run the tests with the gui again:
```sh
BITCOIND=bitcoin-qt ./test/functional/test_runner.py feature_includeconf feature_config_args
ACKs for top commit:
hebasto:
ACK fa6f402bde146f92ed131e0c9c8e15a55e723307
ryanofsky:
utACK fa6f402bde146f92ed131e0c9c8e15a55e723307. Only changes since last review are removing more includes and adding Node::initError method to avoid accessing node `InitError` function and global variables from GUI code.
Tree-SHA512: bd19e08dcea4019dfe40356bc5c63cb583cefed54b6c9dcfb82f1b5b00308d8e2b363549afcaea5e93bf83864dbe0917400c3b70f43a8a5bdff45c9cd34cc294
4f050b91c706181084b9288b8a87b7b637e4e4f7 move-onlyish: move CCoinsViewErrorCatcher out of init.cpp (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal
---
This change moves `CCoinsViewErrorCatcher` out of `init` and into `coins` so that it can later be included in [a `CoinsView` instance](91284964ef (diff-349fbb003d5ae550a2e8fa658e475880R504)) under `CChainState`.
Instead of hardcoding read failure behavior that has knowledge of qt, it accepts error callbacks via `AddReadErrCallback()`.
ACKs for top commit:
dongcarl:
re-ACK 4f050b91c706181084b9288b8a87b7b637e4e4f7
ryanofsky:
utACK 4f050b91c706181084b9288b8a87b7b637e4e4f7. Only change since last review is fixing const.
Tree-SHA512: eaba21606d15d2b8d0e3db7cec57779ce181af953db1ef4af80a0bc1dfb57923d0befde9d61b7be55c32224744f7fb6bd47d4e4c72f3ccfe6eaf0f4ae3765c17
The "version" record that these functions read and write are not
used anywhere in the code except for one place. There is no reason
to expose these functions publicly. Furthermore, this avoids potential
confusion as developers may mistake these functions for actually
reading and writing the wallet version when they do not.
nFileVersion is not the actual file version and is not used except
in one place. So it is removed from CWalletScanState and changed so
that it is just read at the place it is needed. Furthermore, the
"version" record now only indicates the version of the highest
versioned client that has opened a wallet file so the variable
name is changed accordingly
When building the ZMQ static library, add AM_CPPFLAGS to the library
CPPFLAGS. Otherwise, we may miss important flags that are specified
elsewhere. For instance, if --enable-debug is passed and
-DDEBUG_LOCKORDER set, then that would not apply to the ZMQ library
before (causing potential for hard-to-find bugs).
and into coins.cpp. This move is necessary so that we can later include a
CCoinsViewErrorCatcher instance under CChainState.
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
50cede3f5a4d4fbfbb7c420b94e661a6a159bced [mempool] Allow one extra single-ancestor transaction per package (Matt Corallo)
Pull request description:
This implements the proposed policy change from [1], which allows
certain classes of contract protocols involving revocation
punishments to use CPFP. Note that some such use-cases may still
want some form of one-deep package relay, though even this alone
may greatly simplify some lightning fee negotiation.
[1] https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html
ACKs for top commit:
ajtowns:
ACK 50cede3f5a4d4fbfbb7c420b94e661a6a159bced -- looked over code again, compared with previous commit, compiles, etc.
sdaftuar:
ACK 50cede3f5a4d4fbfbb7c420b94e661a6a159bced
ryanofsky:
utACK 50cede3f5a4d4fbfbb7c420b94e661a6a159bced. Changes since last review: adding EXTRA_DESCENDANT_TX_SIZE_LIMIT constant, changing max ancestor size from 1,000,000 to nLimitAncestorSize constant (101,000), fixing test comment and getting rid of unused test node.
Tree-SHA512: b052c2a0f384855572b4579310131897b612201214b5abbb225167224e4f550049e300b471dbf320928652571e92ca2d650050b7cf39ac92b3bc1d2bcd386c1c
