d6b3640ac7 [test] walletcreatefundedpsbt: check RBF is disabled when -walletrbf=0 (Sjors Provoost)
9ed062b568 [doc] rpc: remove "fallback to" from RBF default help (Sjors Provoost)
4fcb698bc2 [rpc] walletcreatefundedpsbt: use wallet default RBF (Sjors Provoost)
Pull request description:
The `walletcreatefundedpsbt` RPC call currently ignores `-walletrbf` and defaults to not use RBF. This PR fixes that.
This PR also replaces UniValue in `ConstructTransaction` with a `bool` in preparation of moving this helper method out of the RPC codebase entirely. This may be a bit overkill, but does slightly simplify it.
Fixes#15878
ACKs for top commit:
achow101:
Code Review ACK d6b3640ac7
l2a5b1:
re-ACK d6b3640
MarcoFalke:
ACK d6b3640ac7
Tree-SHA512: 55b9bccd1ef36b54f6b34793017dc0721103099ad3761b3b04862291ee13d6915915d4dbb1a8567924fa56e5e95dfe10eec070e06701610e70c87f8ea92b2a00
Opening Bitcoin with Qt Creator via the Makefile.am generates a Makefile.am.user. Would be handy to have this file ignored. Looking around I can see this file has snuck into a few downstream projects. I do personally find myself editing commits to remove this file when I've not been paying attention. There's got to have been a few PRs with this file accidentally attached.
IsCoinBase check is already performed early by
AcceptToMemoryPoolWorker
GetDepthInMainChain check is already perfomed by
BroadcastTransaction
To avoid deadlock we MUST keep lock order in
ResendWalletTransactions and CommitTransaction,
even if we lock cs_main again further.
in BroadcastTransaction. Lock order will need
to be clean at once in a future refactoring
Access through a broadcastTransaction method.
Add a wait_callback flag to turn off race protection when wallet
already track its tx being in mempool
Standardise highfee, absurdfee variable name to max_tx_fee
We drop the P2P check in BroadcastTransaction as g_connman is only
called by RPCs and the wallet scheduler, both of which are initialized
after g_connman is assigned and stopped before g_connman is reset.
7a0c224289 Suppress output in test_bitcoin for expected errors (Gert-Jaap Glasbergen)
Pull request description:
Closes#15944
This adds two methods to noui, that allows temporarily suppressing (and then resuming) the output from `noui`. For situations where errors are expected, it's confusing for the test binary to output an error and then conclude with `No errors detected`.
It also uses this supress/reconnect in the tests that currently produce verbose errors when running `test_bitcoin`.
Output of `test_bitcoin` on current master:
```
gertjaap@gjdesktop:~/src/bitcoin$ src/test/test_bitcoin
Running 351 test cases...
Error: Specified -walletdir "/tmp/test_common_Bitcoin Core/1561389554_943311758/tempdir/path_does_not_exist" does not exist
Error: Specified -walletdir "/tmp/test_common_Bitcoin Core/1561389554_643733972/tempdir/not_a_directory.dat" is not a directory
Error: Specified -walletdir "wallets" is a relative path
*** No errors detected
```
Output after this code is merged:
```
gertjaap@gjdesktop:~/src/bitcoin$ src/test/test_bitcoin
Running 351 test cases...
*** No errors detected
```
ACKs for top commit:
l2a5b1:
ACK 7a0c224 - tested and reviewed.
laanwj:
ACK 7a0c224289
Tree-SHA512: c7881f7a431a065329360ffa9937ce4742694c646c90c019d3aff95dfd7fccbdcda9116c5762feb6dfd1108d14f9fb386e203b173c4bde9093afb2b8c977d13d
fac2e6a604 test: Fail early on disconnect in mininode.wait_for_* (MarcoFalke)
Pull request description:
The node might crash or disconnect when our mininode waits for data. Due to the crash, the data is guaranteed to never arrive and we can fail early with an assert
ACKs for top commit:
laanwj:
ACK fac2e6a604
Tree-SHA512: 32ca844eb66bd70ea49103d51c76b953242b1886e0834d96fca8840fc984ff40346d0a799adf8f76b03514a783cb9cec69d45e00bdd328c5192c31b5d8d17af2
b078067b9c gui: Remove unused RPCConsole::tabFocus (João Barbosa)
Pull request description:
Added in #14573 but not used, so begone.
ACKs for top commit:
practicalswift:
utACK b078067b9c
hebasto:
ACK b078067b9c
laanwj:
ACK b078067b9c, there's nothing really to test here
Tree-SHA512: 237276dea4d174b5fca34855447146f79c3faaae7179f4245c70e2070b49282d95f886b1be6d2a33713c81a254f4483a4e4bf850053a8dcb18a3a897bd3da08e
c5d3787367 Allow createwallet to take empty passwords to make unencrypted wallets (Andrew Chow)
Pull request description:
Allow createwallet to take the empty string as a password and interpret that as leaving the wallet unencrypted. Also warn when that happens.
This fixes a bug where it was not possible to use the `avoid_reuse` option for new unencrypted wallets without using named arguments.Thus this allows more `createwallet` options to be added that can be set on unencrypted wallets when using positional arguments.
ACKs for top commit:
jnewbery:
code review ACK c5d3787367
meshcollider:
re-utACK c5d3787367
ryanofsky:
utACK c5d3787367. Changes since last review are rebasing, concatenating warning strings to avoid discarding warnings, adding release notes, and choosing an unambiguous wallet name for the test.
Tree-SHA512: 146737a728dd614ba94d4b166b27e8c9e195badd1709ccab2315afe59176d9b493dfba9b61c3ed81090f059c7e464d709deb06d99451b9a3fff667f527d6f7c9
0b1f4b3c66 wallet: Drop unused OldKey (João Barbosa)
Pull request description:
Replaces #16494, `OldKey` (previously `CWalletKey`) was never serialized in the code history which means that unserialization support is not required, so remove the code entirely.
ACKs for top commit:
jnewbery:
ACK 0b1f4b3c66
laanwj:
ACK 0b1f4b3c66
fanquake:
ACK 0b1f4b3c66
Tree-SHA512: 92e9b2d6fc41f2765492d5d69d18fc4302c40ab44f28c8c30ca652c72767fbc484848c51a38ecf1f447849767a583c398784408bb5f64f9c86f9a5872b325ffc
fa2f991fa2 doc: add note on precedence of options in bitcoin.conf (Torkel Rogstad)
Pull request description:
This was a good addition to the docs, but the PR was closed. So I've cherry-picked the commit and fixed up Russ's comments as well as the linter issue.
ACKs for top commit:
laanwj:
ACK fa2f991fa2
hebasto:
ACK fa2f991fa2, I have reviewed the code and it looks OK, I agree it can be merged.
jamesob:
ACK fa2f991fa2
jonatack:
ACK fa2f991fa2
ryanofsky:
ACK fa2f991fa2. Only suggested changes since previous review.
Tree-SHA512: d8e7bac19e85ad32205652c3c3036766c611cae52e6e3e8af66a2da054659d914dc153d0cf4ace9c0fa7b41f2a8d74d0edd8d83fe7e984b93d70c01a388cf8ec
faf8318c55 test: Split fundrawtx test into subtests (MarcoFalke)
fa6fba3bc8 test: Make local symbols in run_test members (MarcoFalke)
Pull request description:
This prevents scope-leak of symbols that are supposed to be local to one test case.
Top commit has no ACKs.
Tree-SHA512: 9b2a4ca2cdd631ef915d2f7e6cd62375df9a0919448350aa6e5ae4aa8a8fe3ba53870f7a9a25a57736894b4e3a45e861018253ed2d57d9a64c2bb65fa270fad8
9bc8b28c1d refactor : use RelayTransaction in BroadcastTransaction utility (Antoine Riard)
Pull request description:
Implementing suggestion in https://github.com/bitcoin/bitcoin/pull/15713#discussion_r306571420.
Seems a reason of these node utilities is to glue with already there functions, so we should reuse them.
ACKs for top commit:
MarcoFalke:
ACK 9bc8b28c1d
promag:
ACK 9bc8b28c1d, verified there are no more `PushInventory(CInv(MSG_TX, ...`, nice refactor, 👍 @amitiuttarwar.
jnewbery:
ACK 9bc8b28c1d
jonatack:
ACK 9bc8b28c1d, second @jnewbery's suggestions, my guess is they could be added without risking delaying this PR.
Tree-SHA512: 841c65d5f0d9ead5380814bb2260d7ebf03f2a9bfa58a1025785d353bdb42f9122cc257993e6a7bd2bd3f2c74db19c5978cc14be0d83258124ca22e33d6d0164
73b692b531 doc: Add release note for the deprecated totalFee option of bumpfee (João Barbosa)
Pull request description:
Adds release notes for #15996.
Top commit has no ACKs.
Tree-SHA512: 2d75c2fbdd122aa02e808013dd3424843495038bac289b64acc6cc9889bb8ee30d6a91ec2a1b61e6949b1b6e4437331388588d804c81a83757d35d07bf579bc3
0646ca5ea2 Changes the verbosity of msbuild from quiet to normal in the appveyor script. Increasing the verbosity helps to identify the cause of build errors which is the main purpose of the appveyor script. (Aaron Clauson)
Pull request description:
Increasing the verbosity helps to identify the cause of build errors which is the main purpose of the appveyor script.
Partially in response to #16487 where the msbuild error is difficult to determine due to the `quiet` logging level.
ACKs for top commit:
practicalswift:
utACK 0646ca5ea2
MarcoFalke:
ACK 0646ca5ea2. Previously I had to ping sipsorcery every time an issue appeared, now I might be able to look it up myself.
Tree-SHA512: 28d505e3d370523058d6b55ac72fdafd89b451fdc3295e19500dc10a1d868487c62907d86befd0723f263d258a2917ad940b0350cb8e2e0a77799c8c7aa17ec6
80ba4241a6 extract min & max depth onto coin control (Amiti Uttarwar)
Pull request description:
- Refactor `AvailableCoins` to pull min & max depths from coin control.
- Add `m_max_depth` to coin control to support this.
- Addresses issue https://github.com/bitcoin/bitcoin/issues/15823, see thread for further details.
ACKs for top commit:
laanwj:
ACK 80ba4241a6
Tree-SHA512: 8f7c0aa90b3bc3667baf6741b1da2829f3919e1df92ae097d86c6b239f0c024eb410d7100e6251ea8fc49d022fb5a1214bf79b0f8b0014945b7784b2311647d1
05b56d1c93 [wallet] Remove CMerkleTx serialization logic (John Newbery)
783a76f23b [wallet] Flatten CWalletTx class hierarchy (John Newbery)
b3a9d179f2 [wallet] Move CMerkleTx functions into CWalletTx (John Newbery)
Pull request description:
CMerkleTx is only used as a base class for
CWalletTx. It was previously also used for vtxPrev which
was removed in 93a18a3650.
This PR moves all of the CMerkleTx members and logic
into CWalletTx. The CMerkleTx class is kept for deserialization
and serialization of old wallet files.
This makes the refactor in #15931 cleaner.
ACKs for top commit:
laanwj:
ACK 05b56d1c93. Looks good to me.
Tree-SHA512: 3d3a0069ebb536b12a328f1261e7dc55158a71088d445ae4b4ace4142c432dc296f58c8183b1922e54a60b8cc77e9d17c3dce7478294cd68693594baacf2bab3
0000ff0aa7 txmempool: Remove unused default value MemPoolRemovalReason::UNKNOWN (MarcoFalke)
Pull request description:
The `remove*` methods set the removal reason to `UNKNOWN` by default. This is nowhere used; Except in tests, where the value doesn't matter. Fix that by removing the confusing default.
ACKs for top commit:
practicalswift:
utACK 0000ff0aa7
promag:
ACK 0000ff0aa7.
jonasschnelli:
utACK 0000ff0aa7
Tree-SHA512: ffc8b35dd3291a81225171577c743c8bb2645638cab02960b6361174cb68afd739aaab7ab8661d65de5750d37daf16bb7eee9338958d8609093a8d46c2ada1ab
CMerkleTx only exists as a base class for CWalletTx and for wallet file
serialization/deserialization. Move CMerkleTx methods into CWalletTx,
but leave class hierarchy and serialization logic in place.
0c78e49be3 tests: Switch one of the Travis jobs to an unsigned char environment (-funsigned-char) (practicalswift)
Pull request description:
Switch one of the Travis jobs to an unsigned char environment (`-funsigned-char`).
This will help us catch errors due to code written under the assumption that `char` has the same value range as `signed char`.
The signedness of `char` is implementation-defined.
Example:
```
$ uname -a
Linux […] x86_64 x86_64 x86_64 GNU/Linux
$ cat foo.cpp
#include <iostream>
int main() {
char c;
std::cin >> c;
int i = (unsigned char)c;
std::cout << i << "\n";
}
$ clang++ -o foo foo.cpp
$ echo -e "\xff" | ./foo
255
$ clang++ -fsigned-char -o foo foo.cpp
$ echo -e "\xff" | ./foo
255
$ clang++ -funsigned-char -o foo foo.cpp
$ echo -e "\xff" | ./foo
255
$ cat bar.cpp
#include <iostream>
int main() {
char c;
std::cin >> c;
int i = c;
std::cout << i << "\n";
}
$ clang++ -o bar bar.cpp
$ echo -e "\xff" | ./bar
-1
$ clang++ -fsigned-char -o bar bar.cpp
$ echo -e "\xff" | ./bar
-1
$ clang++ -funsigned-char -o bar bar.cpp
$ echo -e "\xff" | ./bar
255
```
`gcc` chars:
* signed: alpha, hppa, ia64, m68k, mips, sh, sparc, x86
* unsigned: arm, powerpc, s390
About `-funsigned-char`:
> Let the type "char" be unsigned, like "unsigned char".
>
> Each kind of machine has a default for what "char" should be. It is either like "unsigned char" by default or like "signed char" by default.
>
> Ideally, a portable program should always use "signed char" or "unsigned char" when it depends on the signedness of an object. But many programs have been written to use plain "char" and expect it to be signed, or expect it to be unsigned, depending on the machines they were written for.
>
> This option, and its inverse, let you make such a program work with the opposite default. The type "char" is always a distinct type from each of "signed char" or "unsigned char", even though its behavior is always just like one of those two.
ACKs for top commit:
laanwj:
ACK 0c78e49be3
Tree-SHA512: ba04590415c0bb9a0bbd348623e57068f75274f53da7247d5c5ecad82e365a5b45893a4a491d318e82a8feb6a25f019d46e01990afb33162e2c9740d33a343d7
29ee4c417d Specify AM_CPPFLAGS for ZMQ. (Daniel Kraft)
Pull request description:
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).
ACKs for top commit:
laanwj:
utACK 29ee4c417d
Tree-SHA512: 64085d71ed3f435a6e4df6dc42bda8b6159a4d292d0547c5b38c09d6ac95e976ad1728cd65278bffdd57363f60a58eb762b1171dafbe055cf94ffcd4f66da877
e0324c3768 Updated python command in readme so it will work on systems that have both python2 and 3 installed. (Aaron Clauson)
Pull request description:
Trivial doc update to the msvc build readme. It updates the python command to use the `py` python launcher so that it will work where Python2 & 3 are installed and 2 is the default (the msvc generator script is incompatible with Python 2).
Top commit has no ACKs.
Tree-SHA512: d7028d1ce4f3132e6b03a02f07ab2464eb946b603e0d46ef5c64882f3a99283602cf61f60e0c3a9c2836767b03222c8f37a9e7bfafda329cf7083f79235b2c65
914923d125 Add setting as known type (Peter Bushnell)
Pull request description:
When loading old wallets I get "Unknown wallet records" showing up in the log file. The key that is adding to the unknown record count is "setting", this is a known key removed in the 0.6 release of Bitcoin in the commit linked below. The "setting" key is not known to the wallet anymore, like "acentry" which is not added as an unknown record, but the "setting" key was used in previous versions of Bitcoin.
972060ce0e (diff-8094838580e1bb7a3bb8fc78dcebc733)
ACKs for top commit:
laanwj:
ACK 914923d125, this code change is straightforward enough and I don't think it makes sense to warn about this key being present.
meshcollider:
ACK 914923d125
Tree-SHA512: 6346690c05cebae2dcd868512322bf5250f6fbd07abb5e747065444185d3f69e19e1a99e3f38d6e34535ffd6979b2297100ba9c7da8e45ca792598eded5ae0d3
faa88d0b5c doc: update labels in CONTRIBUTING.md (MarcoFalke)
Pull request description:
None of the examples in the "trivial" area are acceptable pull requests, unless they are acceptable in a different area (like "doc" or "log").
Fix that by removing the "trivial" area.
ACKs for top commit:
jonatack:
ACK faa88d0b5c
fanquake:
ACK faa88d0b5c - agree that trivial was pretty useless and that the meaning was unclear. Other changes look fine. Surprised the white space linter hasn't been having a field day in this file.
Tree-SHA512: 6208bcc7c84ad0ca6aeaa2de1901c9da8971aac332b5e7a1194ea7b24fb2d887f988aa22fdfa818e89cbcfd8cb8595ce312525f88c81c5ade484fd7c9bd13d1b
fa6f22bf44 wallet: Rename CWalletKey to OldKey (MarcoFalke)
fa6dc7fa5f wallet: Enumerate walletdb keys (MarcoFalke)
Pull request description:
It is nice to see all the keys that exists in a single enum
Also, rename CWalletKey to OldKey and update the outdated documentation
ACKs for top commit:
laanwj:
ACK fa6f22bf44, I'm a big fan of this kind of change as it prevents typos, which can happen with 'magic' strings in the code.
promag:
ACK fa6f22bf44. @jnewbery suggestions are great followups, I think this is good enough.
meshcollider:
utACK fa6f22bf44
achow101:
Code review ACK fa6f22bf44
fanquake:
ACK fa6f22bf44 - I had a quick look over, definitely prefer this to strings floating around everywhere.
Tree-SHA512: 8ac3abd5a0d22dac1d77b8f97fe1e16c2608d650f3e9d6dd1df2fd5aeb35ef6643dfd4cd5c162404bb0100343c927d66df04dc695507ffc84a6c667e603acc54
62d3f5057f qa: fix deprecated log.warn in feature_dbcrash test (Jon Atack)
Pull request description:
This clears up the following deprecation message when running test/functional/feature_dbcrash.py:
```
test/functional/feature_dbcrash.py:270:
DeprecationWarning: The 'warn' method is deprecated, use 'warning' instead
self.log.warn("Node %d never crashed during utxo flush!", i)
```
Git grepping indicates that this was the last remaining use of `log.warn` in the functional tests.
ACKs for top commit:
fanquake:
ACK 62d3f5057f - checked that there were no more occurrences.
Tree-SHA512: 2fe87400f82488e44391f4897876003a98736013e819a7dbc3b3e87a5ffbfba8d5ccab81cf2b7577f40135c95e4db96e93bb8cb24de396efb4ad814fbda09559
This clears up the following deprecation message when running the test:
```
test/functional/feature_dbcrash.py:270: DeprecationWarning: The 'warn' method is deprecated, use 'warning' instead
self.log.warn("Node %d never crashed during utxo flush!", i)
```
Git grepping indicates that this was the last remaining use of `log.warn` in the functional tests.
42a5e912ee [mempool] log correct messages when CPFP fails (John Newbery)
Pull request description:
Fixes a logging issue introduced in #15681
ACKs for top commit:
laanwj:
ACK 42a5e912ee (+utACK from bluematt that isn't registered because it has no commit id)
Tree-SHA512: ff5f423cc4d22838eea00c5b1d39ceda89cd61474c72f256a97c698eb0ec3f2156a97139f537669376132902c1e3943bf84c356a4b98a9a306b4ec57302c2761
