Commit Graph

36079 Commits

Author SHA1 Message Date
Hennadii Stepanov
c39619eeb4
clang-tidy: Fix readability-redundant-string-init in headers
See https://clang.llvm.org/extra/clang-tidy/checks/readability/redundant-string-init.html
2022-12-15 21:24:14 +00:00
Andrew Chow
ba47a4ba97
Merge bitcoin/bitcoin#26668: wallet: if only have one output type, don't perform "mixed" coin selection
89c1491d35 wallet: if only have one output type, don't perform "mixed" coin selection (furszy)

Pull request description:

  For wallets that only have one output type, we are currently performing the same
  selection process over the same coins twice.

  The "mixed coin selection" doesn't add any value to the result
  (there is nothing to mix if the available coins struct has only one type).

ACKs for top commit:
  achow101:
    ACK 89c1491d35
  john-moffett:
    ACK 89c1491d35
  kristapsk:
    cr utACK 89c1491d35

Tree-SHA512: 672eaeed3ba911d13fa61a46f719c8fe1ebe4d2dc7d723040e71937c693659411bc99cdbd9f0014e836b70eebeff1b8ca861f4d81d39e6f79f437364a526edbe
2022-12-14 16:16:03 -05:00
MarcoFalke
678889e6c6
Merge bitcoin/bitcoin#26689: test: add add_wallet_options to TestShell
bcb7123406 test: add add_wallet_options to TestShell (josibake)

Pull request description:

  following 555519d082, `TestShell` now always runs with `-disablewallet`. simple fix is to add `add_wallet_options` to `add_options`; looks like testshell was overlooked when adding in the `add_wallet_options` call to the functional tests in #26480

ACKs for top commit:
  amitiuttarwar:
    ACK bcb7123406

Tree-SHA512: db554a8b3c8ff5bd10cab9592b316035a92f86a0a0ae8ff914de9687bbbb6fc2235bdf82c4cd40e4071782f8b6edf91aad372e82ed3b826c9d8ae39dbe3dbf57
2022-12-14 09:16:02 +01:00
Andrew Chow
daf881de9d
Merge bitcoin/bitcoin#23319: rpc: Return fee and prevout (utxos) to getrawtransaction
f86697163e rpc: Return fee and prevout(s) to getrawtransaction (Douglas Chimento)

Pull request description:

  Add fee response in BTC to getrawtransaction #23264

  ### For Reviewers

  * Verbose arg is now an int
  * Verbose = 2 includes a `fee` field and `prevout`
  * [./test/functional/rpc_rawtransaction.py](./test/functional/rpc_rawtransaction.py) contains a new test to validate fields of new verbosity 2 (not the values)

  ```
  bitcoin-cli -chain=test getrawtransaction 9ae533f7da9be4a34997db78343a8d8d6d6186b6bba3959e56f416a5c70e7de4 2 000000000000001d442e556146d5f2841d85150c200e8d8b8a4b5005b13878f6
  ```
  ```
    "in_active_chain": true,
    "txid": "9ae533f7da9be4a34997db78343a8d8d6d6186b6bba3959e56f416a5c70e7de4",
    "hash": "7f23e3f3a0a256ddea1d35ffd43e9afdd67cc68389ef1a804bb20c76abd6863e",
   ....
    "vin": [
      {
        "txid": "23fc75d6d74f6f97e225839af69ff36a612fe04db58a4414ec4828d1749a05a0",
        "vout": 0,
        "scriptSig": {
          "asm": "",
          "hex": ""
        },
        "prevout": {
          "generated": false,
          "height": 2099486,
          "value": 0.00017764,
          "scriptPubKey": {
            "asm": "0 7846ce1ced3253d8bd43008db2ca364cc722f5a2",
            "hex": "00147846ce1ced3253d8bd43008db2ca364cc722f5a2",
            "address": "tb1q0prvu88dxffa302rqzxm9j3kfnrj9adzk49mlp",
            "type": "witness_v0_keyhash"
          }
        },
        "sequence": 4294967295
      },
  ...
   "fee": 0.00000762
  }
  ```

ACKs for top commit:
  achow101:
    ACK f86697163e
  aureleoules:
    ACK f86697163e
  hernanmarino:
    re ACK f86697163e
  pablomartin4btc:
    re-tACK f86697163e

Tree-SHA512: 591fdc285d74fa7803e04ad01c7b70bc20fac6b1369e7bd5b8e2cde9b750ea52d6c70d79225b74bef4f4bbc0fb960877778017184e146119da4a55f9593d1224
2022-12-13 18:09:09 -05:00
Hennadii Stepanov
ffa32ab108
Merge bitcoin-core/gui#682: Don't directly delete abandoned txes from GUI
e75d227632 Minor fix: Don't directly delete abandoned txes (John Moffett)

Pull request description:

  This fully closes bitcoin/bitcoin#12179. Currently, when a user abandons a transaction by clicking "Abandon Transaction" in the context menu, a call is made to remove it from the GUI view:

  `model->getTransactionTableModel()->updateTransaction(hashQStr, CT_UPDATED, false);`

  (The `false` parameter is for `bool showTransaction`)

  This behavior is probably unwanted, as the transaction is not actually removed from the wallet and would show up again if the node is restarted.

  However, the previous line, `model->wallet().abandonTransaction(hash);`, changes the underlying model and calls `NotifyTransactionChanged(wtx.GetHash(), CT_UPDATED);`, which queues a signal that eventually calls back to `updateTransaction`, this time with `showTransaction` set to `true`. This runs on a separate thread, so it gets called *after* the 'subsequent' `updateTransaction`. The transaction gets removed from the GUI and immediately added back.

  In a nutshell, `updateTransaction` gets called twice. The first (direct) call deletes the transaction from the GUI. The second (sent via a queued signal) brings it back to the GUI. The first direct call is redundant and unwanted. Worse, if the `abandonTransaction` call fails for any reason, the transaction still gets removed from the GUI. (This is what caused bitcoin#12179. It can still be triggered if, eg., a user clicks "Abandon Transaction" the moment after a new block is found.)

  There are no conditions (to my knowledge) where an abandoned transaction should be directly removed from the GUI. If the underlying model changes, the deletion should be reflected anyway by the queued signal to `updateTransaction`.

  The behavior is borne out by the QT logs. To reproduce, send a transaction with RBF enabled, then bump the fee, then 'abandon transaction' on the first transaction. The logs will show something like this:

  ```
  2022-11-28T14:48:00Z [qt] GUI: "NotifyTransactionChanged: 2c5811484f1adec92a739a5e70b453b03eaed0f7cc0538fbd0ee1589e586b951 status= 1"
  2022-11-28T14:48:00Z [qt] GUI: "TransactionTablePriv::updateWallet: 2c5811484f1adec92a739a5e70b453b03eaed0f7cc0538fbd0ee1589e586b951 1"
  2022-11-28T14:48:00Z [qt] GUI: "    inModel=1 Index=381-382 showTransaction=0 derivedStatus=2"
  2022-11-28T14:48:00Z [qt] GUI: "TransactionTablePriv::updateWallet: 2c5811484f1adec92a739a5e70b453b03eaed0f7cc0538fbd0ee1589e586b951 1"
  2022-11-28T14:48:00Z [qt] GUI: "    inModel=0 Index=381-381 showTransaction=1 derivedStatus=0"
  ```

  Notice the duplicate `updateWallet` calls with different `showTransaction` values.

ACKs for top commit:
  hebasto:
    ACK e75d227632
  jarolrod:
    tACK e75d227632

Tree-SHA512: 00f150f747c2ee1605af861a21d5c3b9773a4a9985e8dab62e48bd32885b1bfa4e8cbf805ad61af77aec9d3ccefaed3f4311a29086aa8c22d55d5326ba68ece6
2022-12-13 21:51:06 +00:00
Andrew Chow
8f3021155e
Merge bitcoin/bitcoin#26643: wallet: Move fee underpayment check to after all fee has been set
798430d127 wallet: Sanity check fee paid cannot be negative (Andrew Chow)
c1a84f108e wallet: Move fee underpayment check to after fee setting (Andrew Chow)
e5daf976d5 wallet: Rename nFeeRet in CreateTransactionInternal to current_fee (Andrew Chow)

Pull request description:

  Currently the fee underpayment check occurs right after we calculate what the transaction's fee should be. However the fee paid by the transaction at that time does not always match. Notably, when doing SFFO, the fee paid at that time will almost always be less than the fee required, which then required having a bypass of the underpayment check that results in SFFO payments going through when they should not.

  This PR moves the underpayment check to after fees have been finalized so that we always check whether the fee is being underpaid. This removes the exception for SFFO and unifies this behavior for both SFFO and non-SFFO txs.

ACKs for top commit:
  S3RK:
    Code review ACK 798430d127
  furszy:
    Code review ACK 798430d
  glozow:
    utACK 798430d127, code looks correct to me

Tree-SHA512: 720e8a3dbdc9937b12ee7881eb2ad58332c9584520da87ef3080e6f9d6220ce8d3bd8b9317b4877e56a229113437340852976db8f64df0d5cc50723fa04b02f0
2022-12-13 14:19:00 -05:00
MarcoFalke
a4baf3f177
Merge bitcoin/bitcoin#26628: RPC: Reject RPC requests with same named parameter specified multiple times
8c3ff7d52a test: Suggested cleanups for rpc_namedparams test (Ryan Ofsky)
d1ca563825 bitcoin-cli: Make it an error to specify the "args" parameter two different ways (Ryan Ofsky)
6bd1d20b8c rpc: Make it an error server-side to specify same named parameter multiple times (Ryan Ofsky)
e2c3b18e67 test: Add RPC tests for same named parameter specified more than once (Ryan Ofsky)

Pull request description:

  Make the JSON-RPC server reject requests with the same named parameter specified multiple times, instead of silently overwriting earlier parameter values with later ones.

  Generally JSON keys are supposed to unique, and their order isn't supposed to be significant, so having the server silently discard duplicate keys is error-prone. Most likely if an RPC client is sending a request with duplicate keys it means something is wrong with the request and there should be an error.

  After this change, named parameters are still allowed to specified multiple times on the `bitcoin-cli` command line, since `bitcoin-cli` automatically replaces earlier values with later values before sending the JSON-RPC request. This makes sense, since it's not unusual for the order of command line options to be significant or for later command line options to override earlier ones.

ACKs for top commit:
  MarcoFalke:
    review ACK 8c3ff7d52a 🗂
  kristapsk:
    ACK 8c3ff7d52a
  stickies-v:
    ACK 8c3ff7d52

Tree-SHA512: 2d1357dcc2c171da287aeefc7b333ba4e67babfb64fc14d7fa0940256e18010a2a65054f3bf7fa1571b144d2de8b82d53076111b5f97ba29320cfe84b6ed986f
2022-12-13 17:57:23 +01:00
fanquake
968f03e65c
Merge bitcoin/bitcoin#26477: validation: fix broken maxtipage comparison
e4be0e9b06 test: add -maxtipage test for the maximum allowable value (James O'Beirne)
a451e832b4 fix: validation: cast now() to seconds for maxtipage comparison (James O'Beirne)

Pull request description:

  Since faf44876db, the maxtipage comparison in IsInitialBlockDownload() has been broken, since the NodeClock::now() time_point is in the system's native denomination (nanoseconds).

  Without this patch, specifying the maximum allowable -maxtipage (9223372036854775807) results in a SIGABRT crash:

  ```
  % gdb --args ./src/bitcoind -maxtipage=9223372036854775207 -minimumchainwork=0x00 -stopatheight=30000
  ...
  2022-11-09T15:55:17Z [dnsseed] dnsseed thread exit
  [Thread 0x7fff937fe640 (LWP 69883) exited]

  Thread 29 "b-msghand" received signal SIGABRT, Aborted.
  [Switching to Thread 0x7fff91ffb640 (LWP 69886)]
  __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44
  44      ./nptl/pthread_kill.c: No such file or directory.
  (gdb) bt
  #0  __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44
  #1  0x00007ffff768989f in __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78
  #2  0x00007ffff763da52 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
  #3  0x00007ffff7628469 in __GI_abort () at ./stdlib/abort.c:79
  #4  0x00007ffff7cf79a4 in __mulvdi3 () from /lib/x86_64-linux-gnu/libgcc_s.so.1
  #5  0x00005555558d13ab in std::chrono::__duration_cast_impl<std::chrono::duration<long, std::ratio<1l, 1000000000l> >, std::ratio<1000000000l, 1l>, long, false, true>::__cast<long, std::ratio<1l, 1l> > (__d=...) at /usr/include/c++/12/bits/chrono.h:521
  #6  std::chrono::duration_cast<std::chrono::duration<long, std::ratio<1l, 1000000000l> >, long, std::ratio<1l, 1l> > (__d=...)
      at /usr/include/c++/12/bits/chrono.h:260
  #7  std::chrono::duration<long, std::ratio<1l, 1000000000l> >::duration<long, std::ratio<1l, 1l>, void> (__d=..., this=<optimized out>)
      at /usr/include/c++/12/bits/chrono.h:514
  #8  std::chrono::operator-<long, std::ratio<1l, 1000000000l>, long, std::ratio<1l, 1l> > (__rhs=..., __lhs=...)
      at /usr/include/c++/12/bits/chrono.h:650
  #9  std::chrono::operator-<NodeClock, std::chrono::duration<long, std::ratio<1l, 1000000000l> >, long, std::ratio<1l, 1l> > (__rhs=...,
      __lhs=...) at /usr/include/c++/12/bits/chrono.h:1020
  #10 Chainstate::IsInitialBlockDownload (this=0x555556071940) at ./src/validation.cpp:1545
  #11 0x00005555556efd1e in operator() (__closure=<optimized out>) at ./src/net_processing.cpp:3369
  #12 (anonymous namespace)::PeerManagerImpl::ProcessMessage (this=0x555556219be0, pfrom=..., msg_type=..., vRecv=..., time_received=...,
      interruptMsgProc=...) at ./src/net_processing.cpp:3369
  #13 0x00005555556f75cc in (anonymous namespace)::PeerManagerImpl::ProcessMessages (this=0x555556219be0, pfrom=<optimized out>,
      interruptMsgProc=std::atomic<bool> = { false }) at ./src/net_processing.cpp:4985
  #14 0x00005555556a83c9 in CConnman::ThreadMessageHandler (this=0x5555560ebc70) at ./src/net.cpp:2014
  #15 0x0000555555c4d5d6 in std::function<void ()>::operator()() const (this=0x7fff91ffadb0) at /usr/include/c++/12/bits/std_function.h:591
  #16 util::TraceThread(std::basic_string_view<char, std::char_traits<char> >, std::function<void ()>) (
      thread_name="0\255\377\221\377\177\000\000\v\000\000\000\000\000\000\000TraceThread\000\000\000\000\000P\255\377\221\377\177\000\000\017\000\000\000\000\000\000\000util/thread.cpp\000\000\000\000\000\000\000\000\000\000ihB鵿6\000\000\000\000\000\000\000\000\260\255\377\221\377\177\000\000\277\211\321UUU\000\000p\324\304UUU\000\000\002\000\000\000\000\000\000\000\240xh\367\377\177\000\000\000\000\000\000\000\000\000\000]\340iUUU\000\000p\274\016VUU\000\000\000\000\000\000\000\000\000\000\300\303iUUU\000\000p\206jUUU", '\000' <repeats 11 times>, "ihB鵿6\200\251!VUU\000\000"..., thread_func=...) at util/thread.cpp:21
  #17 0x000055555569e05d in std::__invoke_impl<void, void (*)(std::basic_string_view<char>, std::function<void()>), char const*, CConnman::Start(CScheduler&, const Options&)::<lambda()> > (__f=<optimized out>) at /usr/include/c++/12/bits/invoke.h:61
  #18 std::__invoke<void (*)(std::basic_string_view<char>, std::function<void()>), char const*, CConnman::Start(CScheduler&, const Options&)::<lambda()> > (__fn=<optimized out>) at /usr/include/c++/12/bits/invoke.h:96
  #19 std:🧵:_Invoker<std::tuple<void (*)(std::basic_string_view<char, std::char_traits<char> >, std::function<void()>), char const*, CConnman::Start(CScheduler&, const Options&)::<lambda()> > >::_M_invoke<0, 1, 2> (this=<optimized out>) at /usr/include/c++/12/bits/std_thread.h:252
  #20 std:🧵:_Invoker<std::tuple<void (*)(std::basic_string_view<char, std::char_traits<char> >, std::function<void()>), char const*, CConnman::Start(CScheduler&, const Options&)::<lambda()> > >::operator() (this=<optimized out>) at /usr/include/c++/12/bits/std_thread.h:259
  #21 std:🧵:_State_impl<std:🧵:_Invoker<std::tuple<void (*)(std::basic_string_view<char, std::char_traits<char> >, std::function<void()>), char const*, CConnman::Start(CScheduler&, const Options&)::<lambda()> > > >::_M_run(void) (this=<optimized out>)
      at /usr/include/c++/12/bits/std_thread.h:210
  #22 0x00007ffff7ad43d3 in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6
  #23 0x00007ffff7687b27 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:435
  #24 0x00007ffff770a78c in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
  (gdb)
  ```

ACKs for top commit:
  MarcoFalke:
    review ACK e4be0e9b06 🏽

Tree-SHA512: d892d6264a284d952a68a8631a6301277373b8df939dafd9e2652f2f22ab60712cde63b90c27c67ea2d05f02443452e3e4e1b9f25479bfaca00d4c4de13b9fbd
2022-12-13 10:07:37 +00:00
josibake
bcb7123406
test: add add_wallet_options to TestShell
without this, testShell runs with -disablewallet
2022-12-12 17:58:15 +01:00
MarcoFalke
6061eb6564
Merge bitcoin/bitcoin#26199: p2p: Don't self-advertise during version processing
956c67059c refactor, doc: Improve SetupAddressRelay call in version processing (Martin Zumsande)
3c43d9db1e p2p: Don't self-advertise during VERSION processing (Gleb Naumenko)

Pull request description:

  This picks up the last commit from #19843.

  Previously, we would prepare to self-announce to a new peer while parsing a `version` message from that peer.
  This is redundant, because we do something very similar in `MaybeSendAddr()`, which is called from `SendMessages()` after
  the version handshake is finished.

  There are a couple of differences:

  1) `MaybeSendAddr()` self-advertises to all peers we do address relay with, not just outbound ones.
  2) `GetLocalAddrForPeer()` called from `MaybeSendAddr()` makes a probabilistic decision to either advertise what they think we are or what we think we are, while `PushAddress()` on `version` deterministically only does the former if the address from the latter is unroutable.
  3) During `version` processing, we haven't received a potential sendaddrv2 message from our peer yet, so self-advertisements with addresses from addrV2-only networks would always be dropped in `PushAddress()`.

  Since it's confusing to have two slightly different mechanisms for self-advertising, and the one in `MaybeSendAddr()` is better, remove the one in `version`.

ACKs for top commit:
  stratospher:
    ACK  956c670
  naumenkogs:
    ACK 956c67059c
  amitiuttarwar:
    reACK 956c67059c

Tree-SHA512: 933d40615289f055c022170dde7bad0ac0a1d4be377538bfe9ba64375cfeb03bcd803901591f0739ac4850c880e8475a68fd1ab0330800030ab7f19e38c00274
2022-12-12 10:12:09 +01:00
MarcoFalke
1ea02791f3
Merge bitcoin/bitcoin#26666: refactor: Deleted unreachable code in httpserver.cpp
8f5c560e11 refactor: Refactored RequestMethodString function to follow developer notes (JoaoAJMatos)
7fd3b9491b refactor: Deleted unreachable code in httpserver.cpp (JoaoAJMatos)

Pull request description:

  Some of the code in httpserver.cpp was unreachable, and didn't follow the developer notes.
  Continuation of [#26570 ](https://github.com/bitcoin/bitcoin/pull/26570)

ACKs for top commit:
  stickies-v:
    re-ACK [8f5c560](8f5c560e11)

Tree-SHA512: ba8cf4c6dde9e2bb0ca9d63a0de86dfa37b070803dde71ac8384c261045835697a2335652cf5894511b3af8fd99f30e1cbda4e4234815b8b39538ade90fab3f9
2022-12-10 13:03:22 +01:00
fanquake
e1fb7381be
Merge bitcoin/bitcoin#26672: build: Update libmultiprocess library
1986f129c6 build: Update `libmultiprocess` library (Hennadii Stepanov)

Pull request description:

  This update in particular includes:
  - https://github.com/chaincodelabs/libmultiprocess/pull/78 which is [required](https://github.com/bitcoin/bitcoin/pull/25972#issuecomment-1246812573) for https://github.com/bitcoin/bitcoin/pull/25972
  - https://github.com/chaincodelabs/libmultiprocess/pull/74
  - https://github.com/chaincodelabs/libmultiprocess/pull/70

ACKs for top commit:
  ryanofsky:
    Code review ACK 1986f129c6

Tree-SHA512: 2d9fa72df5de7d5be37d77d479702cba36c45e9fa9d9fc27e58aac0437c39e4a1054c0ac53b612cb43e830982e444d98c7d3e651d093ac68344e66f4734227bb
2022-12-10 10:40:40 +00:00
fanquake
a28fb36c47
Merge bitcoin/bitcoin#26673: univalue: Remove confusing getBool method
293849a260 univalue: Remove confusing getBool method (Ryan Ofsky)

Pull request description:

  Drop `UniValue::getBool` method because it is easy to confuse with the `UniValue::get_bool` method, and could potentially cause bugs. Unlike `get_bool`, `getBool` doesn't ensure that the value is a boolean and returns false for all integer, string, array, and object values instead of throwing an exception.

  The `getBool` method is also redundant because it is an alias for `isTrue`. There were only 5 `getBool()` calls in the codebase, so this commit replaces them with `isTrue()` or `get_bool()` calls as appropriate.

  These changes were originally made by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/26213 but were dropped to limit the scope of that PR.

ACKs for top commit:
  justinpickering:
    ACK 293849a260
  sipa:
    utACK 293849a260
  w0xlt:
    ACK 293849a260
  hebasto:
    ACK 293849a260, also verified that the removed `getBool` method is not mentioned in any docs:
  furszy:
    ACK 293849a2

Tree-SHA512: 9fbfe5e2083410f123b18703a0cc0161ecbbb4958f331c9ff808dcfcc6ad499b0e896abd16fb8ea200c53ba29878db9812ce141e59cc5e0fd174741b0bcb192d
2022-12-10 10:18:18 +00:00
fanquake
3b5fb6e77a
Merge bitcoin/bitcoin#26213: rpc: Strict type checking for RPC boolean parameters
fa0153e609 refactor: Replace isTrue with get_bool (MarcoFalke)
fa2cc5d1d6 bugfix: Strict type checking for RPC boolean parameters (MarcoFalke)

Pull request description:

ACKs for top commit:
  ryanofsky:
    Code review ACK fa0153e609
  furszy:
    Code ACK fa0153e6

Tree-SHA512: b221f823c69d90c94447fd491071ff3659cfd512872b495ebc3e711f50633351974102c9ef7e50fa4a393c4131d349adea8fd41cc9d66f1f31e1f5e7a5f78757
2022-12-10 09:58:33 +00:00
Andrew Chow
798430d127 wallet: Sanity check fee paid cannot be negative
We need to check that the fee is not negative even before it is
finalized. The setting of fees for SFFO may adjust the fee to be
"correct" and no longer negative, but erroneously reduce the amounts too
far. So we need to check this condition before we do those adjustments.
2022-12-09 14:52:43 -05:00
Andrew Chow
c1a84f108e wallet: Move fee underpayment check to after fee setting
It doesn't make sense to be checking whether the fee paid is underpaying
before we've finished setting the fees. So do that after we have done
the reduction for SFFO and change adjustment for fee overpayment.
2022-12-09 14:52:26 -05:00
JoaoAJMatos
8f5c560e11 refactor: Refactored RequestMethodString function to follow developer notes
Removed the default case in the switch statement in order to comply with the Developer Notes
2022-12-09 16:14:27 +00:00
JoaoAJMatos
7fd3b9491b refactor: Deleted unreachable code in httpserver.cpp
Removed all break statements from both RequestMethodString and GetRequestMethod functions as they were unreachable
2022-12-09 16:13:57 +00:00
MarcoFalke
9e229a542f
Merge bitcoin/bitcoin#26601: test: Move wallet tests to wallet_*.py
fa7d71accc test: Move rpc_fundrawtransaction.py to wallet_fundrawtransaction.py (MarcoFalke)
fa933d6985 test: Move feature_backwards_compatibility.py to wallet_backwards_compatibility.py (MarcoFalke)

Pull request description:

  The tests only tests the wallet and it doesn't make sense to extend it for other stuff, so clarify that.

ACKs for top commit:
  fanquake:
    ACK fa7d71accc
  pablomartin4btc:
    re-ACK fa7d71accc

Tree-SHA512: 9dc131ed8ff119bd6d43d04ecc5c96d02f2d6fdc3d0805492774a414c0fbe6a984da7631154122a080c54ddd25fc5a5bdd52b282c918fce4932057ba72c2bf71
2022-12-09 16:34:48 +01:00
Ryan Ofsky
8c3ff7d52a test: Suggested cleanups for rpc_namedparams test
No changes in behavior, just implements review suggestions from

https://github.com/bitcoin/bitcoin/pull/19762#discussion_r1025573943
https://github.com/bitcoin/bitcoin/pull/19762#discussion_r1035955247
https://github.com/bitcoin/bitcoin/pull/26628#discussion_r1038765894
2022-12-09 10:34:28 -05:00
Hennadii Stepanov
1986f129c6
build: Update libmultiprocess library
Replacing `install` with `install-lib` and `install-bin` is not strictly
necessary just to update the library, but it takes advantage of recent
changes in the new version, and makes the build more minimal.
2022-12-09 15:26:58 +00:00
Ryan Ofsky
293849a260 univalue: Remove confusing getBool method
Drop UniValue::getBool method because it is easy to confuse with the
UniValue::get_bool method, and could potentially cause bugs. Unlike get_bool,
getBool doesn't ensure that the value is a boolean and returns false for all
integer, string, array, and object values instead of throwing an exceptions.

The getBool method is also redundant because it is an alias for isTrue. There
were only 5 getBool() calls in the codebase, so this commit replaces them with
isTrue() or get_bool() calls as appropriate.

These changes were originally made by MarcoFalke in
https://github.com/bitcoin/bitcoin/pull/26213 but were dropped to limit the
scope of that PR.

Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
2022-12-09 10:03:26 -05:00
MarcoFalke
fa7d71accc
test: Move rpc_fundrawtransaction.py to wallet_fundrawtransaction.py 2022-12-09 11:54:29 +01:00
MarcoFalke
fa933d6985
test: Move feature_backwards_compatibility.py to wallet_backwards_compatibility.py 2022-12-09 11:54:17 +01:00
MarcoFalke
16624e6ff3
Merge bitcoin/bitcoin#26660: test: Use last release in compatibility tests
fabb24cbef test: Use last release in compatibility tests (MarcoFalke)

Pull request description:

  In compatibility tests it makes sense to always use the last release without the new feature, as it is likely more in use than any even older previous release.

ACKs for top commit:
  Sjors:
    utACK fabb24c

Tree-SHA512: beb854f4d28ba313282e1e0303abb0e09377828b138bde5a3e209337210b6b4c24855ab90a68f8789387001e4ca33b15cc37dbc9b7809929f4e7d1b69833a527
2022-12-09 09:25:52 +01:00
MarcoFalke
6d11f19cf5
Merge bitcoin/bitcoin#26658: test: Fix backwards compatibility intermittent failure
c29bff5b91 test: Fix backwards compatibility intermittent failure (Aurèle Oulès)

Pull request description:

  Fixes #24400.

  See https://github.com/bitcoin/bitcoin/issues/24400#issuecomment-1341531696 to reproduce the failure.

  As MarcoFalke suggested in https://github.com/bitcoin/bitcoin/issues/24400#issuecomment-1342282165, it can happen that the wallet is not fully flushed before being copied over to the other node. Fixed by unloading the wallet before copying the file.

ACKs for top commit:
  Sjors:
    utACK c29bff5

Tree-SHA512: ea93b859878873d07fa44ccb1c5b4c1b1014be8568dc2b9bbef02bb7a9230fefa7a71b149eb553870d3f4e986263342ea4f996fa0221098a2244f38952100471
2022-12-09 09:21:54 +01:00
furszy
89c1491d35
wallet: if only have one output type, don't perform "mixed" coin selection
there is nothing to mix.
2022-12-08 15:56:36 -03:00
fanquake
3eaf7be6ad
Merge bitcoin/bitcoin#24279: build: Make $(package)_*_env available to all $(package)_*_cmds
affbf58a1e build: Move environment variables into `$(package)_config_env` (Hennadii Stepanov)
d44fcd3c97 build: Make $(package)_*_env available to all $(package)_*_cmds (Hennadii Stepanov)

Pull request description:

  On master (1e7564eca8) the depends build system, which is based on pure GNU Make, works, but it lacks robustness, and in some corner cases it fails. For example, see bitcoin/bitcoin#22552.

  Another [bug](https://github.com/bitcoin/bitcoin/issues/22719) in the depends build system has already become a problem at least two times in the past (https://github.com/bitcoin/bitcoin/pull/16883#issuecomment-683817472 and https://github.com/bitcoin/bitcoin/pull/24134). Each time the problem was solved with other means.

  The initial [solution](https://github.com/bitcoin/bitcoin/pull/19882) had some discussion. Also it was discussed on the IRC meeting in #bitcoin-core-builds channel. This PR, actually, is a resurrection of it, as the bug silently struck pretty [recently](https://github.com/bitcoin/bitcoin/pull/24134).

  The bug is well described in bitcoin/bitcoin#22719.

  Here is another, a bit simpler description, which requires only basic shell (bash, dash etc) experience.
  After creating targets by this code:1e7564eca8/depends/funcs.mk (L280) a "draft" line of recipe like `$($(1)_config_env) $(call $(1)_config_cmds, $(1))` becomes a shell command sequence `VAR1=foo VAR2=bar command1 && command2` which is supposed to be executed in a [new sub-shell](https://www.gnu.org/software/make/manual/html_node/Execution.html#Execution).

  Please note that `VAR1=foo VAR2=bar` part is visible for the first `command1` only (for details see shell docs). Example:
  ```sh
  $ VAR1="foo" VAR2="bar" echo "begin" && printenv VAR1 && printenv VAR2 && echo "end"
  begin
  $ echo $?
  1
  ```

  Using the `export` command is a trivial solution:
  ```sh
  $ export VAR1="foo" VAR2="bar"; echo "begin" && printenv VAR1 && printenv VAR2 && echo "end"
  begin
  foo
  bar
  end
  $ echo $?
  0
  ```

  As a [new sub-shell](https://www.gnu.org/software/make/manual/html_node/Execution.html#Execution) is invoked for each line of the recipe, there are no side effects of using `export`. It means this solution should not be considered invasive.

  Fixes bitcoin/bitcoin#22719.

  ---

  Also this PR removes no longer needed crutch from `qt.mk`.

ACKs for top commit:
  fanquake:
    ACK affbf58a1e

Tree-SHA512: 0ce2cf82870a7774bdf1592fac50857126ae47da902e349f1092d50109223be9d6a8efd5e92ec08c2ca775b17516482aabaf232378950ade36484a883acc177b
2022-12-08 16:41:40 +00:00
MarcoFalke
5126e625cb
Merge bitcoin/bitcoin#26378: refactor: Pass reference to last header, not pointer
fa579f3063 refactor: Pass reference to last header, not pointer (MacroFake)

Pull request description:

  It is never a nullptr, otherwise an assertion would fire in UpdatePeerStateForReceivedHeaders.

  Passing a reference makes the code easier to read and less brittle.

ACKs for top commit:
  john-moffett:
    ACK fa579f3
  aureleoules:
    ACK fa579f3063

Tree-SHA512: 9725195663a31df57ae46bb7b11211cc4963a8f3d100f60332bfd4a3f3327a73ac978b3172e3007793cfc508dfc7c3a81aab57a275a6963a5ab662ce85743fd0
2022-12-08 17:04:05 +01:00
fanquake
07ac7a2dbf
Merge bitcoin/bitcoin#26513: Make static nLastFlush and nLastWrite Chainstate members
07dfbb5bb8 Make static nLastFlush and nLastWrite Chainstate members (Aurèle Oulès)

Pull request description:

  Fixes #22189.

  The `static std::multimap<uint256, FlatFilePos> mapBlocksUnknownParent; ` referenced in the issue was already fixed by #25571. I don't believe Chainstate references any other static variables.

ACKs for top commit:
  jamesob:
    ACK 07dfbb5bb8 ([`jamesob/ackr/26513.1.aureleoules.make_static_nlastflush_a`](https://github.com/jamesob/bitcoin/tree/ackr/26513.1.aureleoules.make_static_nlastflush_a))
  theStack:
    Concept and code-review ACK 07dfbb5bb8

Tree-SHA512: 0f26463c079bbc5e0e62707d4ca4c8c9bbb99edfa3391d48d4915d24e2a1190873ecd4f9f11da25b44527671cdc82c41fd8234d56a4592a246989448d34406b0
2022-12-08 15:35:28 +00:00
MarcoFalke
fabb24cbef
test: Use last release in compatibility tests 2022-12-08 14:57:25 +01:00
Aurèle Oulès
c29bff5b91
test: Fix backwards compatibility intermittent failure 2022-12-08 11:13:34 +01:00
MarcoFalke
1801d8c3c9
Merge bitcoin/bitcoin#26308: rpc/rest/zmq: reduce LOCK(cs_main) scope: ~6 times as many requests per second
d7f61e7d59 rpc: reduce LOCK(cs_main) scope in gettxoutproof (Andrew Toth)
4d92b5aaba rpc: reduce LOCK(cs_main) scope in GetUndoChecked and getblockstats (Andrew Toth)
efd82aec8a rpc: reduce LOCK(cs_main) scope in blockToJSON (Andrew Toth)
f00808e932 rpc: reduce LOCK(cs_main) scope in GetBlockChecked and getblock (Andrew Toth)
7d253c943f zmq: remove LOCK(cs_main) from NotifyBlock (Andrew Toth)
c75e3d2772 rest: reduce LOCK(cs_main) scope in rest_block (Andrew Toth)

Pull request description:

  Picking up from #21006.

  After commit ccd8ef65f9 it is no longer required to hold `cs_main` when calling `ReadBlockFromDisk`. This can be verified in `master` at https://github.com/bitcoin/bitcoin/blob/master/src/node/blockstorage.cpp#L755. Same can be seen for `UndoReadFromDisk` https://github.com/bitcoin/bitcoin/blob/master/src/node/blockstorage.cpp#L485.

  The first commit moves `ReadBlockFromDisk` outside the lock scope in `rest_block`, where we can see a huge performance improvement when fetching blocks with multiple threads.

  My test setup, on an Intel i7 with 8 cores (16 threads):

  1. Start a fully synced bitcoind, with this `bitcoin.conf`:
  ```
      rest=1
      rpcthreads=16
      rpcworkqueue=64
      rpcuser=user
      rpcpassword=password
  ```
  2. Run ApacheBench: 10000 requests, 16 parallel threads, fetching block nr. 750000 in binary:
  ```
      ab -n 10000 -c 16 "http://127.0.0.1:8332/rest/block/0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e.bin"
  ```

  Time per request (mean)
      183 ms on master
      30 ms this branch

  So this can process 6.1 times as many requests, and saturates all the cores instead of keeping them partly idle waiting in the lock. With 8 threads the mean times were 90 ms on master and 19 ms on this branch, a speedup of 4.7x.

  Big thanks to martinus for finding this and the original PR.

  The second commit is from a suggestion on the original PR by jonatack to remove the unnecessary `LOCK(cs_main)` in the zmq notifier's `NotifyBlock`.

  I also found that this approach could be applied to rpcs `getblock` (including `verbosity=3`), `getblockstats`, and `gettxoutproof` with similar very good results. The above benchmarks steps need to be modified slightly for RPC. Run the following ApacheBench command with different request data in a file named `data.json`:
  ```
  ab -p data.json -n 10000 -c 16 -A user:password "http://127.0.0.1:8332/"
  ```
  For `getblock`, use the following in `data.json`:
  ```
  {"jsonrpc": "1.0", "id": "curltest", "method": "getblock", "params": ["0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e"]}
  ```
  master - 184 ms mean request time
  branch - 28 ms mean request time

  For `getblock` with verbosity level 3, use the following in `data.json`:
  ```
  {"jsonrpc": "1.0", "id": "curltest", "method": "getblock", "params": ["0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e", 3]}
  ```
  This verbosity level fetches an undo file from disk, so it benefits from this approach as well. However, a lot of time is spent serializing to JSON so the performance gain is not as severe.
  master - 818 ms mean request time
  branch - 505 ms mean request time

  For `getblockstats`, use the following in `data.json`:
  ```
  {"jsonrpc": "1.0", "id": "curltest", "method": "getblockstats", "params": ["0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e", ["minfeerate","avgfeerate"]]}
  ```
  This request used a lock on reading both a block and undo file, so the results are very good.
  master - 244 ms mean request time
  branch - 28 ms mean request time

ACKs for top commit:
  MarcoFalke:
    re-ACK d7f61e7d59 💫
  hebasto:
    ACK d7f61e7d59, I have reviewed the code and it looks OK. Did not make benchmarking though.

Tree-SHA512: 305ac945b4571c5f47646d4f0e78180d7a3d40b2f70ee43e4b3e00c96a465f6d0b9c750b8e85c89ed833e557e2cdb5896743f07ef90e4e53d4ad85452b545886
2022-12-08 10:48:02 +01:00
Andrew Chow
a653f4bb1f
Merge bitcoin/bitcoin#25934: wallet, rpc: add label to listsinceblock
4e362c2b72 doc: add release note for 25934 (brunoerg)
fe488b4c4b test: add coverage for `label` in `listsinceblock` (brunoerg)
722e9a418d wallet, rpc: add `label` to `listsinceblock` (brunoerg)
852891ff98 refactor, wallet: use optional for `label` in `ListTransactions` (brunoerg)

Pull request description:

  This PR adds `label` parameter to `listsinceblock` to be able to fetch all incoming transactions having the specified label since a specific block.

  It's possible to use it in `listtransactions`, however, it's only possible to set the number of transactions to return, not a specific block to fetch from. `getreceivedbylabel` only returns the total amount received, not the txs info. `listreceivedbylabel` doesn't list all the informations about the transactions and it's not possible to fetch since a block.

ACKs for top commit:
  achow101:
    ACK 4e362c2b72
  w0xlt:
    ACK 4e362c2b72
  aureleoules:
    ACK 4e362c2b72

Tree-SHA512: fbde5db8cebf7a27804154fa61997b5155ad512e978cebb78c17acab9efcb624ea5f39d649899d12e5e675f80d4d0064cae8132b864de0d93a8d1e6fbcb9a737
2022-12-07 18:42:41 -05:00
Andrew Chow
bbfcbcfa0c
Merge bitcoin/bitcoin#24611: Add fish completions
ccba4fe7e3 doc: Add completion subdir to contrib/README.md (willcl-ark)
7075848f96 script: Add fish completions (willcl-ark)
a27a445b71 refactor: Sub-folder bash completions (willcl-ark)

Pull request description:

  The completions are dynamically generated from the respective binary
  help pages.

  Completions should be sourced into the shell or added to
  `$XDG_CONFIG/fish/completions`. See [where to put completions](https://fishshell.com/docs/current/completions.html#where-to-put-completions) for more information.

  As the completions are auto-generated they should only require as much maintenance as the bash equivalents, which is to say very little!

ACKs for top commit:
  achow101:
    ACK ccba4fe7e3
  josibake:
    ACK ccba4fe7e3

Tree-SHA512: fe6ed899ea1fe90f82970bde7739db11dd0c845ccd70b65f28ad5212f75b57d9105a3a7f70ccdff552d5b21fa3fe9c697d128fb10740bae31fe1854e716b4b8b
2022-12-07 18:30:14 -05:00
MarcoFalke
fa0153e609
refactor: Replace isTrue with get_bool
This makes the code more robust, see previous commit.

In general replacing isTrue with get_bool is not equivalent because
get_bool can throw exceptions, but in this case, exceptions won't happen
because of RPCTypeCheck() and isNull() checks in the preceding code.
2022-12-07 17:56:49 +01:00
MarcoFalke
fa2cc5d1d6
bugfix: Strict type checking for RPC boolean parameters 2022-12-07 17:55:58 +01:00
Hennadii Stepanov
affbf58a1e
build: Move environment variables into $(package)_config_env 2022-12-07 16:51:48 +00:00
Hennadii Stepanov
d44fcd3c97
build: Make $(package)_*_env available to all $(package)_*_cmds 2022-12-07 16:51:35 +00:00
MarcoFalke
9052d869c9
Merge bitcoin/bitcoin#26517: test: Changed small_txpuzzle_randfee to return the virtual size instead of the transaction hex for feerate calculation.
6fb102c9f3 test: Changed small_txpuzzle_randfee to return the virtual size instead of the transaction hex for feerate calculation. (Randall Naar)

Pull request description:

  The fee rates used in feature_fee_estimation.py are calculated using the raw transaction size instead of the virtual transaction size (which is used in 'CBlockPolicyEstimator::processBlockTx' and 'CBlockPolicyEstimator::processBlock'). This leads to inconsistencies as the fee rates used in check_raw_estimates are incorrect and can cause assertions to fail.

  refs #25179

ACKs for top commit:
  MarcoFalke:
    ACK 6fb102c9f3

Tree-SHA512: b2bca85fffa605aeb17574f050736d4556506d782dd7fd969e165e48e108fd95ef4c4e2abbae83cce05ca777a00f4459cabfa0932694fa8bb93ddfba09d84357
2022-12-07 17:33:37 +01:00
fanquake
7d51560003
Merge bitcoin/bitcoin#26298: refactor: Move src/interfaces/*.cpp files to libbitcoin_common.a
b19c4124b3 refactor: Rename ambiguous interfaces::MakeHandler functions (Ryan Ofsky)
dd6e8bd71c build: remove BOOST_CPPFLAGS from libbitcoin_util (fanquake)
82e272a109 refactor: Move src/interfaces/*.cpp files to libbitcoin_common.a (Ryan Ofsky)

Pull request description:

  These belong in `libbitcoin_common.a`, not `libbitcoin_util.a`, because they aren't general-purpose utilities, they just contain some common glue code that is used by both the node and the wallet. Another reason not to include these in `libbitcoin_util.a` is to prevent them from being used by the kernel library.

  Also rename ambiguous `MakeHandler` functions to `MakeCleanupHandler` and `MakeSignalHandler`. Cleanup function handler was introduced after boost signals handler, so original naming didn't make much sense.

  This just contains a move-only commit, and a rename commit. There are no actual code or behavior changes.

  This PR is an alternative to #26293, and solves the same issue of removing a boost dependency from the _util_ library. The advantages of this PR compared to #26293 are that it keeps the source directory structure more flat, and it avoids having to change #includes all over the codebase.

ACKs for top commit:
  hebasto:
    ACK b19c4124b3

Tree-SHA512: b3a1d33eedceda7ad852c6d6f35700159d156d96071e59acae2bc325467fef81476f860a8855ea39cf3ea706a1df2a341f34fb2dcb032c31a3b0e9cf14103b6a
2022-12-07 14:54:23 +00:00
MarcoFalke
272fb0a5cf
Merge bitcoin/bitcoin#26645: util: Include full version id in bug reports
fa825bd227 util: Include full version id in bug reports (MarcoFalke)

Pull request description:

  This will show the unique id of the full source code when the bug occurred, which can help debugging

ACKs for top commit:
  1440000bytes:
    utACK fa825bd227
  theStack:
    ACK fa825bd227
  john-moffett:
    ACK fa825bd227

Tree-SHA512: a7a775718f5f9796b5cffafbb3ace8adb5c163414ec584a57143157fc9dfb86f799e3b9c8365fcb831ee1e9eafc59d699d1653d772c68392de421b3de74dcd61
2022-12-07 08:47:32 +01:00
Andrew Chow
e5daf976d5 wallet: Rename nFeeRet in CreateTransactionInternal to current_fee
nFeeRet represents the fee that the transaction currently pays. Update
it's name to reflect that.
2022-12-06 15:18:18 -05:00
Andrew Toth
d7f61e7d59 rpc: reduce LOCK(cs_main) scope in gettxoutproof 2022-12-06 15:07:04 -05:00
Andrew Toth
4d92b5aaba rpc: reduce LOCK(cs_main) scope in GetUndoChecked and getblockstats 2022-12-06 15:07:04 -05:00
Andrew Toth
efd82aec8a rpc: reduce LOCK(cs_main) scope in blockToJSON 2022-12-06 15:07:04 -05:00
Andrew Toth
f00808e932 rpc: reduce LOCK(cs_main) scope in GetBlockChecked and getblock 2022-12-06 15:07:04 -05:00
Andrew Toth
7d253c943f zmq: remove LOCK(cs_main) from NotifyBlock 2022-12-06 15:07:04 -05:00
Andrew Toth
c75e3d2772 rest: reduce LOCK(cs_main) scope in rest_block 2022-12-06 15:07:04 -05:00
Hennadii Stepanov
0596aa40f7
Merge bitcoin-core/gui#683: doc: Drop no longer relevant comment
5d332da2cf doc: Drop no longer relevant comment (Hennadii Stepanov)

Pull request description:

  The comment was introduced in 4cf3411056, and since 7e4bd19785 it has been no longer relevant.

ACKs for top commit:
  jarolrod:
    ACK 5d332da2cf

Tree-SHA512: 6d32561336993b1ff7d7c524d090ac52aefb40078ed706ca4c6d5026cc3f63244c49c0e00e45ff192ba0e9f1527faf63249aa18bc8aa677b9e053d387e0f4027
2022-12-06 18:58:08 +00:00