Commit graph

38827 commits

Author SHA1 Message Date
Ryan Ofsky
f562856d02
Merge bitcoin/bitcoin#27866: blockstorage: Return on fatal flush errors
d8041d4e04 blockstorage: Return on fatal undo file flush error (TheCharlatan)
f0207e0030 blockstorage: Return on fatal block file flush error (TheCharlatan)
5671c15f45 blockstorage: Mark FindBlockPos as nodiscard (TheCharlatan)

Pull request description:

  The goal of this PR is to establish that fatal blockstorage flush errors should be treated as errors at their call site.

  Prior to this patch `FlushBlockFile` may have failed without returning in `Chainstate::FlushStateToDisk`, leading to a potential write from `WriteBlockIndexDB` that may refer to a block that is not fully flushed to disk yet. By returning if either `FlushUndoFile` or `FlushBlockFile` fail, we ensure that no further write operations take place that may lead to an inconsistent database when crashing. Add `[[nodiscard]]` annotations to them such that they are not ignored in future.

  Functions that call either `FlushUndoFile` or `FlushBlockFile`, need to handle these extra abort cases properly. Since `Chainstate::FlushStateToDisk` already produces an abort error in case of `WriteBlockIndexDB` failing, no extra logic for functions calling `Chainstate::FlushStateToDisk` is required.

  Besides `Chainstate::FlushStateToDisk`, `FlushBlockFile` is also called by `FindBlockPos`, while `FlushUndoFile` is only called by `FlushBlockFile` and `WriteUndoDataForBlock`. For both these cases, the flush error is not further bubbled up. Instead, the error is logged and a comment is provided why bubbling up an error would be less desirable in these cases.

  ---

  This pull request is part of a larger effort towards improving the shutdown / abort / fatal error handling in validation code. It is a first step towards implementing proper fatal error return type enforcement similar as proposed by theuni in this pull request [comment](https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1563561502). For ease of review of these critical changes, a first step would be checking that `AbortNode` leads to early and error-conveying returns at its call site. Further work for enforcing returns when `AbortNode` is called is done in https://github.com/bitcoin/bitcoin/pull/27862.

ACKs for top commit:
  stickies-v:
    re-ACK d8041d4
  ryanofsky:
    Code review ACK d8041d4e04

Tree-SHA512: 47ade9b873b15e567c8f60ca538d5a0daf32163e1031be3212a3a45eb492b866664b225f2787c9e40f3e0c089140157d8fd1039abc00c7bdfeec1b52ecd7e219
2023-09-29 13:29:51 -04:00
Andrew Chow
d18a8f6f69
Merge bitcoin/bitcoin#28525: net: Drop v2 garbage authentication packet
e3720bca39 net: Simplify v2 recv logic by decoupling AAD from state machine (Tim Ruffing)
b0f5175c04 net: Drop v2 garbage authentication packet (Tim Ruffing)

Pull request description:

  Note that this is a breaking change, see also https://github.com/bitcoin/bips/pull/1498

  The benefit is a simpler implementation:
   - The protocol state machine does not need separate states for garbage authentication and version phases.
   - The special case of "ignoring the ignore bit" is removed.
   - The freedom to choose the contents of the garbage authentication packet is removed. This simplifies testing.

ACKs for top commit:
  naumenkogs:
    ACK e3720bca39
  sipa:
    ACK e3720bca39. Re-ran the v2 transport fuzzer overnight.
  ajtowns:
    ACK e3720bca39 - simpler and more flexible, nice
  achow101:
    ACK e3720bca39
  Sjors:
    utACK e3720bca39
  theStack:
    Code-review ACK e3720bca39

Tree-SHA512: 16600ed868c8a346828de075c4072e37cf86440751d08ab099fe8b58ff71d8b371a90397d6a4247096796db68794275e7e0403f218859567d04838e0411dadd6
2023-09-29 09:50:02 -04:00
Andrew Chow
9d5150ac47
Merge bitcoin/bitcoin#28540: tests: Fix wallet_resendwallettransactions.py intermittent failure by using manual bumps instead of bumpfee
b5a962564e tests: Use manual bumps instead of bumpfee for resendwallettransactions (Andrew Chow)

Pull request description:

  Bumpfee will try to increase the entire package to the target feerate, which causes repeated bumpfees to quickly shoot up in fees, causing intermittent failures when the fee is too large. We don't care about this property, just that the child is continuously replaced until we observe it's position in mapWallet is before its parent. Instead of using bumpfee, we can create raw transactions which have only pay (just above) the additional incremental relay fee, thus avoiding this problem.

  Fixes #28491

ACKs for top commit:
  kevkevinpal:
    ACK [b5a9625](b5a962564e)
  mzumsande:
    Code review ACK b5a962564e
  pablomartin4btc:
    ACK b5a962564e -> adding the `try_rpc` to avoid (skip) any possible failure around the manual bump fee (if we ever reach it as [explained](https://github.com/bitcoin/bitcoin/pull/28540#issuecomment-1737648048)) makes a lot of sense as the spirit of the test is the tx (child before parent) sort in the `mapWallet` (as also [explained](https://github.com/bitcoin/bitcoin/issues/28491#issuecomment-1736161363)).
  MarcoFalke:
    lgtm ACK b5a962564e

Tree-SHA512: f184f11c73be0c30753181901f51a3b4b9c4135e0c4681e9f4ca94692c49bac15c91683c85266a2124333c8593e9919bfd9102724616faab299740f2eb98741f
2023-09-28 11:22:10 -04:00
Fabian Jahr
f9047771d6
lint: fix custom mypy cache dir setting 2023-09-28 13:20:25 +02:00
glozow
6619d6a8dc
Merge bitcoin/bitcoin#28450: Add package evaluation fuzzer
262ab8ef78 Add package evaluation fuzzer (Greg Sanders)

Pull request description:

  This fuzzer target caught the issue in https://github.com/bitcoin/bitcoin/pull/28251 within 5 minutes on master branch, and an additional issue which I've applied a preliminary patch to cover.

  Fuzzer target does the following:

  1) Picks mempool confgs, including max package size, count, mempool size, etc
  2) Generates 1 to 26 transactions with arbitrary coins/fees, the first N-1 spending only confirmed outpoints
  3) Nth transaction, if >1, sweeps all unconfirmed outpoints in mempool
  4) If N==1, it may submit it through single-tx submission path, to allow for more interesting topologies
  5) Otherwise submits through package submission interface
  6) Repeat 1-5  a few hundred times per mempool instance

  In other words, it ends up building chains of txns in the mempool using parents-and-children packages, which is currently the topology supported on master.

  The test itself is a direct rip of tx_pool.cpp, with a number of assertions removed because they were failing for unknown reasons, likely due to the notification changes of single tx submission to package, which is used to track addition/removal of transactions in the test. I'll continue working on re-adding these assertions for further invariant testing.

ACKs for top commit:
  murchandamus:
    ACK 262ab8ef78
  glozow:
    reACK 262ab8ef78
  dergoegge:
    tACK 262ab8ef78

Tree-SHA512: 190784777d0f2361b051b3271db8f79b7927e3cab88596d2c30e556da721510bd17f6cc96f6bb03403bbf0589ad3f799fa54e63c1b2bd92a2084485b5e3e96a5
2023-09-28 12:05:24 +01:00
Andrew Chow
19a7e608f9
Merge bitcoin/bitcoin#28505: rpc: bumpfee, improve doc for 'reduce_output' arg
b3db8c9d5c rpc: bumpfee, improve doc for 'reduce_output' arg (furszy)

Pull request description:

  Fixes #28180. Resulted from discussions with S3RK, achow101, and Murch.

  The current argument name and description are dangerous as it don't
  describe the case where the user selects the recipient output as the
  change address. This one could end up been increased by the inputs
  minus outputs remainder. Which, when `bumpfee` adds new inputs
  to the transaction, leads the process to send more coins to the
  recipient. Which is not what the user would expect from a
  'reduce_output' param naming.

ACKs for top commit:
  S3RK:
    ACK b3db8c9d5c
  achow101:
    ACK b3db8c9d5c
  murchandamus:
    ACK b3db8c9d5c

Tree-SHA512: 91f607e2f5849041d7c099afdddae11af8bed5b1ac90c9d22921267f272e21b44e107d6968e037f05f958a61fe29e94e5fb44b224fb3606f197f83ec4ba3b1e7
2023-09-27 16:46:27 -04:00
Greg Sanders
262ab8ef78 Add package evaluation fuzzer 2023-09-27 16:27:05 -04:00
Pieter Wuille
6ef405ddb1 key: don't allocate secure mem for null (invalid) key
Instead of storing the key material as an std::vector (with secure allocator),
use a secure_unique_ptr to a 32-byte array, and use nullptr for invalid keys.
This means a smaller CKey type, and no secure/dynamic memory usage for invalid
keys.
2023-09-27 15:05:26 -04:00
Anthony Towns
d9841a7ac6 Add make_secure_unique helper
Co-authored-by: Pieter Wuille <bitcoin-dev@wuille.net>
2023-09-27 15:05:15 -04:00
Andrew Chow
b5a962564e tests: Use manual bumps instead of bumpfee for resendwallettransactions
Bumpfee will try to increase the entire package to the target feerate,
which causes repeated bumpfees to quickly shoot up in fees, causing
intermittent failures when the fee is too large. We don't care about
this property, just that the child is continuously replaced until we
observe it's position in mapWallet is before its parent. Instead of
using bumpfee, we can create raw transactions which have only pay the
additional incremental relay fee, thus avoiding this problem.
2023-09-27 11:39:07 -04:00
MarcoFalke
fa40b3ee22
test: Avoid test failure on Linux root without cap-add LINUX_IMMUTABLE 2023-09-27 16:47:44 +02:00
Tim Ruffing
e3720bca39 net: Simplify v2 recv logic by decoupling AAD from state machine 2023-09-27 12:19:54 +02:00
Tim Ruffing
b0f5175c04 net: Drop v2 garbage authentication packet
See also https://github.com/bitcoin/bips/pull/1498

The benefit is a simpler implementation:
 - The protocol state machine does not need separate states for garbage
   authentication and version phases.
 - The special case of "ignoring the ignore bit" is removed.
 - The freedom to choose the contents of the garbage authentication
   packet is removed. This simplifies testing.
2023-09-27 12:19:54 +02:00
Andrew Chow
782701ce7d test: Test loading wallets with conflicts without a chain
Loading a wallet with conflicts without a chain (e.g. wallet tool and
migration) would previously result in an assertion due to -1 being both
a valid number of conflict confirmations, and the indicator that that
member has not been set yet.
2023-09-26 22:28:53 -04:00
Andrew Chow
4660fc82a1 wallet: Check last block and conflict height are valid in MarkConflicted
MarkConflicted calculates conflict confirmations incorrectly when both
the last block processed height and the conflicting height are negative
(i.e. uninitialized). If either are negative, we should not be marking
conflicts and should exit early.
2023-09-26 21:28:20 -04:00
furszy
b3db8c9d5c
rpc: bumpfee, improve doc for 'reduce_output' arg
The current argument name and description are dangerous as it don't
describe the case where the user selects the recipient output as the
change address. This one could end up been increased by the inputs
minus outputs remainder. Which, when bumpfee adds new inputs
to the transaction, leads the process to send more coins to the
recipient. Which is not what the user would expect from a
'reduce_output' param naming.

Co-authored-by: Murch <murch@murch.one>
2023-09-26 20:17:02 -03:00
fanquake
c9f288244b
Merge bitcoin/bitcoin#28483: refactor: Return CAutoFile from BlockManager::Open*File()
fa56c421be Return CAutoFile from BlockManager::Open*File() (MarcoFalke)
9999b89cd3 Make BufferedFile to be a CAutoFile wrapper (MarcoFalke)
fa389d902f refactor: Drop unused fclose() from BufferedFile (MarcoFalke)

Pull request description:

  This is required for https://github.com/bitcoin/bitcoin/pull/28052, but makes sense on its own, because offloading logic to `CAutoFile` instead of re-implementing it allows to delete code and complexity.

ACKs for top commit:
  TheCharlatan:
    Re-ACK fa56c421be
  willcl-ark:
    tACK fa56c421be

Tree-SHA512: fe4638f3a6bd3f9d968cfb9ae3259c9d6cd278fe2912cbc90289851311c8c781099db4c160e775960975c4739098d9af801a8d2d12603f371f8edfe134d8f85a
2023-09-26 14:01:44 +01:00
fanquake
dcfbf3c210
Merge bitcoin/bitcoin#28512: doc: Be vague instead of wrong about MALLOC_ARENA_MAX
12f7257b8f doc: Be vague instead of wrong about MALLOC_ARENA_MAX (Tim Ruffing)

Pull request description:

  Before this commit, we claim that glibc's malloc implementation uses 2 arenas by default. But that's true only on 32-bit systems, and even there, it uses *up* to 2 arenas.

  This commit fixes the wrong statement. The new statement is intentionally vague to reduce our maintenance burden.

  For details, see:
  https://www.gnu.org/software/libc/manual/html_node/Memory-Allocation-Tunables.html#index-glibc_002emalloc_002earena_005fmax

  Noticed in:
  https://github.com/bitcoin/bitcoin/pull/27642#issuecomment-1728103427

ACKs for top commit:
  fanquake:
    ACK 12f7257b8f

Tree-SHA512: c0ff1e35b682a841e366a1cad26e18ff79a93d97103529be35a972c7dcbb95f5354e7a7b98a86731f491434d64685bb58cc3cc9100f0577d8f75db05e951b09a
2023-09-24 18:54:16 +01:00
fanquake
ac9fa6ec78
Merge bitcoin/bitcoin#28385: [refactor] rewrite DisconnectedBlockTransactions to not use boost
4313c77400 make DisconnectedBlockTransactions responsible for its own memory management (glozow)
cf5f1faa03 MOVEONLY: DisconnectedBlockTransactions to its own file (glozow)
2765d6f343 rewrite DisconnectedBlockTransactions as a list + map (glozow)
79ce9f0aa4 add std::list to memusage (glozow)
59a35a7398 [bench] DisconnectedBlockTransactions (glozow)
925bb723ca [refactor] batch-add transactions to DisconnectedBlockTransactions (glozow)

Pull request description:

  Motivation
  - I think it's preferable to use stdlib data structures instead of depending on boost if we can achieve the same thing.
  - Also see #28335 for further context/motivation. This PR simplifies that one.

  Things done in this PR:
  - Add a bench for `DisconnectedBlockTransactions` where we reorg and the new chain has {100%, 90%, 10%} of the same transactions. AFAIU in practice, it's usually close to 100%.
  - Rewrite `DisconnectedBlockTransactions` as a `std::list` + `unordered_map` instead of a boost multi index container.
    - On my machine, the bench suggests the performance is very similar.
  - Move `DisconnectedBlockTransactions` from txmempool.h to its own kernel/disconnected_transactions.h. This struct isn't used by txmempool and doesn't have much to do with txmempool. My guess is that it's been living there for convenience since the boost includes are there.

ACKs for top commit:
  ismaelsadeeq:
    Tested ACK 4313c77400
  stickies-v:
    ACK 4313c77400
  TheCharlatan:
    ACK 4313c77400

Tree-SHA512: 273c80866bf3acd39b2a039dc082b7719d2d82e0940e1eb6c402f1c0992e997256722b85c7e310c9811238a770cfbdeb122ea4babbc23835d17128f214a1ef9e
2023-09-23 18:42:36 +01:00
Andrew Chow
719cb301e6
Merge bitcoin/bitcoin#28492: RPC: descriptorprocesspsbt returns hex encoded tx if complete
a99e9e655a doc: add release note (ismaelsadeeq)
2b4edf889a test: check `descriptorprocesspsbt` return hex encoded tx (ismaelsadeeq)
c405207a18 rpc: `descriptorprocesspsbt` return hex encoded tx (ismaelsadeeq)

Pull request description:

  Coming from [#28414 comment](https://github.com/bitcoin/bitcoin/pull/28414#pullrequestreview-1618684391) Same thing also for `descriptorprocesspsbt`.

  Before this PR `descriptorprocesspsbt` returns a boolean `complete` which indicates that the psbt is final, users then have to call `finalizepsbt` to get the hex encoded network transaction.

  In this PR if the psbt is complete the return object also has the hex encoded network transaction ready for broadcast with `sendrawtransaction`.

  This save users calling `finalizepsbt` with the descriptor, if it is already complete.

ACKs for top commit:
  achow101:
    ACK a99e9e655a
  pinheadmz:
    ACK a99e9e655a
  ishaanam:
    ACK a99e9e655a

Tree-SHA512: c3f1b1391d4df05216c463127cd593f8703840430a99febb54890bc66fadabf9d9530860605f347ec54c1694019173247a0e7a9eb879d3cbb420f9e8d9839b75
2023-09-23 11:55:38 -04:00
Hennadii Stepanov
b000ed5ee5
Merge bitcoin-core/gui#119: Replace send-to-self with dual send+receive entries
099dbe4224 GUI: TransactionRecord: When time/index/etc match, sort send before receive (Luke Dashjr)
2d182f77cd Bugfix: Ignore ischange flag when we're not the sender (Luke Dashjr)
71fbdb7f40 GUI: Remove SendToSelf TransactionRecord type (Luke Dashjr)
f3fbe99fcf GUI: TransactionRecord: Refactor to turn send-to-self into send+receive pairs (Luke Dashjr)
b9765ba1d6 GUI: TransactionRecord: Use "any from me" as the criteria for deciding whether a transaction is a send or receive (Luke Dashjr)

Pull request description:

  Makes the GUI transaction list more like the RPC, and IMO clearer in general.

  As a side effect, this also fixes the GUI entries when a transaction is a net profit to us, but some inputs were also from us.

  Originally https://github.com/bitcoin/bitcoin/pull/15115

  Has Concept ACKs from @*Empact @*jonasschnelli

ACKs for top commit:
  hebasto:
    ACK 099dbe4224.

Tree-SHA512: 7d581add2f59431aa019126d54232a1f15723def5147d7a1b672e9b6d525b6e5a944cc437701aa1bd5bd0fbe557a3d1f4b239337f42bdba4fe1d3960442d0e3b
2023-09-22 18:29:51 +01:00
Hennadii Stepanov
bce7b087cb
Merge bitcoin-core/gui#739: Disable and uncheck blank when private keys are disabled
9ea31eba04 gui: Disable and uncheck blank when private keys are disabled (Andrew Chow)

Pull request description:

  Unify the GUI's create wallet with the RPC createwallet so that the blank flag is not set when private keys are disabled.

ACKs for top commit:
  S3RK:
    Code review ACK 9ea31eba04
  jarolrod:
    ACK 9ea31eba04
  pablomartin4btc:
    tACK 9ea31eba04

Tree-SHA512: 0c90dbd77e66f088c6a57711a4b91e254814c4ee301ab703807f281cacd4b08712d2dfeac7661f28bc0e93acc55d486a17b8b4a53ffa57093d992e7a3c51f4e8
2023-09-22 15:26:26 +01:00
Hennadii Stepanov
2e1d4bf444
Merge bitcoin-core/gui#755: Silence -Wcast-function-type warning
befb42f146 qt: Silence `-Wcast-function-type` warning (Hennadii Stepanov)

Pull request description:

  On Fedora 38 @ 8f7b9eb871:
  ```
  $ x86_64-w64-mingw32-g++ --version | head -1
  x86_64-w64-mingw32-g++ (GCC) 12.2.1 20221121 (Fedora MinGW 12.2.1-8.fc38)
  $ ./configure CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site CXXFLAGS="-Wno-return-type -Wcast-function-type"
  $ make > /dev/null
  qt/winshutdownmonitor.cpp: In static member function 'static void WinShutdownMonitor::registerShutdownBlockReason(const QString&, HWND__* const&)':
  qt/winshutdownmonitor.cpp:46:42: warning: cast between incompatible function types from 'FARPROC' {aka 'long long int (*)()'} to 'PSHUTDOWNBRCREATE' {aka 'int (*)(HWND__*, const wchar_t*)'} [-Wcast-function-type]
     46 |     PSHUTDOWNBRCREATE shutdownBRCreate = (PSHUTDOWNBRCREATE)GetProcAddress(GetModuleHandleA("User32.dll"), "ShutdownBlockReasonCreate");
        |                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ```

  [Required](https://github.com/bitcoin/bitcoin/pull/25972#issuecomment-1713999563) for https://github.com/bitcoin/bitcoin/pull/25972.

  Picked from https://trac.nginx.org/nginx/ticket/1865.

ACKs for top commit:
  MarcoFalke:
    review ACK befb42f146

Tree-SHA512: b37b2c5dd8702caf84d1833c3511bc46ee14f23b84678b8de0fd04e24e5ecc5fd4d27ba38be0d0b08de91299369f70d4924c895a71fd8e0b6feffcfb7407574a
2023-09-22 12:41:28 +01:00
fanquake
b66f6dcb26
Merge bitcoin/bitcoin#28513: ci: Install Homebrew's pkg-config package
43cd8029fa ci: Install Homebrew's `pkg-config` package (Hennadii Stepanov)

Pull request description:

  Some versions of macOS images lack the `pkg-config` package.

  For example, https://github.com/bitcoin/bitcoin/actions/runs/6248032071/job/16961797066:
  ```
  Runner Image
    Image: macos-13
    Version: 20230417.1
  ```

  ```
  + ./autogen.sh
  configure.ac:16: error: PKG_PROG_PKG_CONFIG macro not found. Please install pkg-config and re-run autogen.sh
  ```

  This PR makes Homebrew install the `pkg-config` package explicitly.

  Also please refer to [macOS Build Guide](https://github.com/bitcoin/bitcoin/blob/master/doc/build-osx.md).

ACKs for top commit:
  kevkevinpal:
    ACK [43cd802](43cd8029fa)
  MarcoFalke:
    lgtm ACK 43cd8029fa
  RandyMcMillan:
    ACK 43cd802

Tree-SHA512: 2b934b22e5f6748634089e0525b92219484e37b2afc11e9fd4c53faf112b33ca1c8deb5b4aa36939bf5c4807e7599d4aabae6335317ecc5d4a4d562bbd7dbdf2
2023-09-21 16:32:59 +00:00
fanquake
f29091410d
Merge bitcoin/bitcoin#28379: Refactor: Remove m_is_test_chain
78c2707b2a Refactor: Replace 'isMockableChain' with inline 'ChainType' check for 'submitpackage' (Tim Neubauer)
27b4084e16 Refactor: Remove m_is_test_chain (Tim Neubauer)

Pull request description:

  Remove the m_is_test_chain bool
  Compiled and run tests locally

  #28376

ACKs for top commit:
  MarcoFalke:
    re-ACK 78c2707b2a
  ajtowns:
    ACK 78c2707b2a

Tree-SHA512: 2eedd855c379dd12b7ff28b0e03414680cc892313f16502f36e09906513df9c222e8cc2cea3ff4d9a4f47c9efdfa00d017f38398021b0c96d4543711206d6ff8
2023-09-21 16:31:05 +00:00
Andrew Chow
2303fd2f43
Merge bitcoin/bitcoin#28471: Fix virtual size limit enforcement in transaction package context
eb8f58f5e4 Add functional test to catch too large vsize packages (Greg Sanders)
1a579f9d01 Handle over-sized (in virtual bytes) packages with no in-mempool ancestors (Greg Sanders)
bc013fe8e3 Bugfix: Pass correct virtual size to CheckPackageLimits (Luke Dashjr)
533660c58a Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT to avoid vbyte confusion (Greg Sanders)

Pull request description:

  (Alternative) Minimal subset of https://github.com/bitcoin/bitcoin/pull/28345 to:

  1) Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT which accounts for additional WU necessary to not exclude default chain limit transactions that would have been accepted individually. Avoids sigops vbyte confusion.
  2) pass correct vsize to chain limit evaluations in package context
  3) stop overly-large packages that have no existing mempool ancestors (also a bugfix by itself if someone sets non-standard chain limits)

  This should fix the known issues while not blocking additional refactoring later.

ACKs for top commit:
  achow101:
    ACK eb8f58f5e4
  ariard:
    Re-Code ACK eb8f58f5e
  glozow:
    reACK eb8f58f5e4

Tree-SHA512: 1b5cca1a526207e25d387fcc29a776a3198c3a013dc2b35c6275b9d5a64db2476c154ebf52e3a1aed0b9924c75613f21a946577aa760de28cadf0c9c7f68dc39
2023-09-21 12:13:52 -04:00
fanquake
cf0711cac3
Merge bitcoin/bitcoin#27934: test: added coverage to estimatefee
d05be124db test: added coverage to estimatefee (kevkevin)

Pull request description:

  Added a assert for an rpc error when we try to estimate fee for the max conf_target

  Line I am adding coverage to https://github.com/bitcoin/bitcoin/blob/master/src/rpc/fees.cpp#LL71C52-L71C52

ACKs for top commit:
  MarcoFalke:
    lgtm ACK d05be124db

Tree-SHA512: dfab075989446e33d1a5ff1a308f1ba1b9f80cce3848fbe4231f69212ceef456a3f2b19365a42123e0397c31893fd9f1fd9973cc00cfbb324386e12ed0e6bccc
2023-09-21 15:50:15 +00:00
Andrew Chow
41cb17fdb6
Merge bitcoin/bitcoin#28078: net, refactor: remove unneeded exports, use helpers over low-level code, use optional
4ecfd3eaf4 Inline short, often-called, rarely-changed basic CNetAddr getters (Jon Atack)
5316ae5dd8 Convert GetLocal() to std::optional and remove out-param (Jon Atack)
f1304db136 Use higher-level CNetAddr and CNode helpers in net.cpp (Jon Atack)
07f5891588 Add CNetAddr::IsPrivacyNet() and CNode::IsConnectedThroughPrivacyNet() (Jon Atack)
df488563b2 GetLocal() type-safety, naming, const, and formatting cleanups (stickies-v)
fb4265747c Add and use CNetAddr::HasCJDNSPrefix() helper (Jon Atack)
5ba73cd0ee Move GetLocal() declaration from header to implementation (Jon Atack)
11426f6557 Move CaptureMessageToFile() declaration from header to implementation (Jon Atack)
deccf1c484 Move IsPeerAddrLocalGood() declaration from header to implementation (Jon Atack)

Pull request description:

  and other improvements noticed while reviewing #27411.

  Addresses https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1263969104 and https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1263967598.

  See commit messages for details.

ACKs for top commit:
  achow101:
    ACK 4ecfd3eaf4
  vasild:
    ACK 4ecfd3eaf4
  stickies-v:
    ACK 4ecfd3eaf4

Tree-SHA512: d792bb2cb24690aeae9bedf97e92b64fb6fd080c39385a4bdb8ed05f37f3134d85bda99da025490829c03017fd56382afe7951cdd039aede1c08ba98fb1aa7f9
2023-09-21 11:26:16 -04:00
Andrew Chow
5027d41988
Merge bitcoin/bitcoin#26366: rpc, test: addnode improv + add test coverage for invalid command
f52cb02f70 doc: make it clear that `node` in `addnode` refers to the node's address (brunoerg)
effd1efefb test: `addnode` with an invalid command should throw an error (brunoerg)
56b27b8487 rpc, refactor: clean-up `addnode` (brunoerg)

Pull request description:

  This PR:

  - Adds test coverage for an invalid `command` in `addnode`.
  - Rename `test_getaddednodeinfo` to `test_addnode_getaddednodeinfo` and its log since this function also tests `addnode` and it doesn't worth to split into 2 ones.
  - Makes it clear in docs that `node` in `addnode` refers to the node's address. It seemed a little weird for me "The node (see getpeerinfo for nodes)", it could mean a lot of things e.g. the node id.
  - Some small improv/clean-up: use `const` where possible, rename some vars, and remove the check for nullance for `command` since it's a non-optional field.

ACKs for top commit:
  achow101:
    ACK f52cb02f70
  jonatack:
    ACK f52cb02f70
  theStack:
    re-ACK f52cb02f70

Tree-SHA512: e4a69e58b784e233463945b4d55a401957f9fe4562c129f59216a44f44fb3221d3449ac578fb35e665ca654c6ade2e741b72c3df78040f7527229c77b6c5b82e
2023-09-21 06:35:16 -04:00
Hennadii Stepanov
1d4846a844
Merge bitcoin-core/gui#738: Add menu option to migrate a wallet
48aae2cffe gui: Add File > Migrate Wallet (Andrew Chow)
577be889cd gui: Optionally return passphrase after unlocking (Andrew Chow)
5b3a85b4c6 interfaces, wallet: Expose migrate wallet (Andrew Chow)

Pull request description:

  GUI users need to be able to migrate wallets without going to the RPC console.

ACKs for top commit:
  jarolrod:
    ACK 48aae2cffe
  pablomartin4btc:
    tACK 48aae2cffe
  hebasto:
    ACK 48aae2cffe

Tree-SHA512: 2d02b1e85e7d6cfbf503f417f150cdaa0c63822942e9a6fe28c0ad3e7f40a957bb01a375c909a60432dc600e84574881aa446c7ec983b56f0bb23f07ef15de54
2023-09-20 22:32:31 +01:00
Hennadii Stepanov
43cd8029fa
ci: Install Homebrew's pkg-config package
Some versions of macOS images lack the 'pkg-config' package, which is
required for the build process.
2023-09-20 21:49:57 +01:00
Andrew Chow
8247a8db69
Merge bitcoin/bitcoin#28154: test: refactor: deduplicate segwitv0 ECDSA signing for tx inputs
83d7cfd542 test: refactor: deduplicate segwitv0 ECDSA signing for tx inputs (Sebastian Falbesoner)

Pull request description:

  This PR is a simple follow-up for #28025. It introduces a `signing_input_segwitv0` helper in order to deduplicate the following steps needed to create a segwitv0 ECDSA signature:
  1. calculate the `SegwitV0SignatureHash` with the desired sighash type
  2. create the actual digital signature by calling ECKey.sign_ecdsa on the signature message hash calculated above
  3. put the DER-encoded result (plus sighash byte) at the bottom of the witness stack

ACKs for top commit:
  achow101:
    ACK 83d7cfd542
  pinheadmz:
    code review ACK at 83d7cfd542

Tree-SHA512: b8e55409ddc9ddb14cfc06daeb4730d7750a4632f175f88dcac6ec4d216e71fd4a7eee325a64d6ebba3b33be50bcd30c2de7400f834c01abb67e52840d9823b6
2023-09-20 13:50:15 -04:00
Tim Ruffing
12f7257b8f
doc: Be vague instead of wrong about MALLOC_ARENA_MAX
Before this commit, we claim that glibc's malloc implementation uses 2
arenas by default. But that's true only on 32-bit systems, and even
there, it uses *up* to 2 arenas.

This commit fixes the wrong statement. The new statement is
intentionally vague to reduce our maintenance burden.

For details, see:
https://www.gnu.org/software/libc/manual/html_node/Memory-Allocation-Tunables.html#index-glibc_002emalloc_002earena_005fmax

Noticed in:
https://github.com/bitcoin/bitcoin/pull/27642#issuecomment-1728103427
2023-09-20 17:12:24 +00:00
Sebastian Falbesoner
96b3f2dbe4 test: add unit test coverage for Python ECDSA implementation 2023-09-20 18:19:29 +02:00
fanquake
99ce8366ed
Merge bitcoin/bitcoin#28504: ci: Use nproc over MAKEJOBS in 01_base_install
fa3b5e5e57 ci: Use nproc over MAKEJOBS in 01_base_install (MarcoFalke)

Pull request description:

  Currently `$MAKEJOBS` is the default value in `01_base_install.sh` when building the container image.

  This problem can't be fixed (see below), so just use `nproc` for now.

  Other solutions would be bad:

  * Passing in the `MAKEJOBS` as a dockerfile env would create a new image if the number of tasks are changed, seems verbose and confusing.
  * Leaving `master` as-is would leave CPUs unused if there are more than `4`.

ACKs for top commit:
  fanquake:
    ACK - I think this is fine as-is, it's an improvement over the current state fa3b5e5e57

Tree-SHA512: 15014eb7b548945737b0fed5cd46f0cd4b72b1d54d044f60fce628f914053a2de6518878428a54822d236d08d6f257d8c464d3fb589400f4be56022d2ec8676d
2023-09-20 16:14:30 +00:00
Greg Sanders
eb8f58f5e4 Add functional test to catch too large vsize packages 2023-09-20 11:33:53 -04:00
Greg Sanders
1a579f9d01 Handle over-sized (in virtual bytes) packages with no in-mempool ancestors 2023-09-20 10:34:09 -04:00
Andrew Chow
ff564c75e7
Merge bitcoin/bitcoin#27511: rpc: Add test-only RPC getaddrmaninfo for new/tried table address count
28bac81a34 test: add functional test for getaddrmaninfo (stratospher)
c8eb8dae51 rpc: Introduce getaddrmaninfo for count of addresses stored in new/tried table (stratospher)

Pull request description:

  implements https://github.com/bitcoin/bitcoin/issues/26907. split off from #26988 to keep RPC, CLI discussions separate.

  This PR introduces a new RPC `getaddrmaninfo`which returns the count of addresses in the new/tried table of a node's addrman broken down by network type. This would be useful for users who want to see the distribution of addresses from different networks across new/tried table in the addrman.

  ```jsx
  $ getaddrmaninfo

  Result:
  {                   (json object) json object with network type as keys
    "network" : {     (json object) The network (ipv4, ipv6, onion, i2p, cjdns)
      "new" : n,      (numeric) number of addresses in new table
      "tried" : n,    (numeric) number of addresses in tried table
      "total" : n     (numeric) total number of addresses in both new/tried tables from a network
    },
    ...
  }
  ```

  ### additional context from [original PR](https://github.com/bitcoin/bitcoin/pull/26988)

  1. network coverage tests were skipped because there’s a small chance that addresses from different networks could hash to the same bucket and cause count of different network addresses in the tests to fail. see https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1137596851.
  2. #26988 uses this RPC in -addrinfo CLI. Slight preference for keeping the RPC hidden since this info will mostly be useful to only super users. see https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1173964808.

ACKs for top commit:
  0xB10C:
    ACK 28bac81a34
  willcl-ark:
    reACK 28bac81a34
  achow101:
    ACK 28bac81a34
  brunoerg:
    reACK 28bac81a34
  theStack:
    Code-review ACK 28bac81a34

Tree-SHA512: 346390167e1ebed7ca5c79328ea452633736aff8b7feefea77460e04d4489059334ae78a3f757f32f5fb7827b309d7186bebab3c3760b3dfb016d564a647371a
2023-09-20 08:25:20 -04:00
Luke Dashjr
bc013fe8e3 Bugfix: Pass correct virtual size to CheckPackageLimits 2023-09-20 08:13:18 -04:00
Greg Sanders
533660c58a Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT to avoid vbyte confusion
While allowing submitted packages to be slightly larger than what
may be allowed in the mempool to allow simpler reasoning
about contextual-less checks vs chain limits.
2023-09-20 08:10:30 -04:00
Andrew Chow
3966b0a0b6
Merge bitcoin/bitcoin#28472: Remove MemPoolAccept::m_limits to avoid mutating it in package evaluation
ee589d4466 Add regression test for m_limit mutation (Greg Sanders)
275579d8c1 Remove MemPoolAccept::m_limits, only have local copies for carveouts (Greg Sanders)

Pull request description:

  Without remoing it, if we ever call `PreChecks()` multiple times for any reason during any one `MempoolAccept`, subsequent invocations may have incorrect limits, allowing longer/larger chains than should be allowed.

  Currently this is only an issue with `submitpackage`, so this is not exposed on mainnet.

ACKs for top commit:
  achow101:
    ACK ee589d4466
  glozow:
    ACK ee589d4466, nits can be ignored
  ariard:
    Code Review ACK ee589d446

Tree-SHA512: 14cf8edc73e014220def82563f5fb4192d1c2c111829712abf16340bfbfd9a85e2148d723af6fd4995d503dd67232b48dcf8b1711668d25b5aee5eab1bdb578c
2023-09-20 07:49:13 -04:00
fanquake
e9a4793b82
Merge bitcoin/bitcoin#28432: build: Produce a .zip for macOS distribution
b5790c35f7 build: remove dmg dependencies (fanquake)
33ae0bd1e4 macdeploy: remove DMG generation from deploy script (fanquake)
a128111c29 build: produce a .zip for macOS distribution (Hennadii Stepanov)
c38561d6b1 build: add -zip option to macdeployqtplus (fanquake)

Pull request description:

  It is https://github.com/bitcoin/bitcoin/pull/27099 revived with addressed [comments](https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1708705686).

  From https://github.com/bitcoin/bitcoin/pull/27099#issue-1584429885:
  > Reviving the discussion around using a `.zip` for the distributed macOS binaries, as opposed to a `.dmg`.
  >
  > Given we only had a single report of the "no finder window" issue (#26176), I wonder if that means macOS users were able to figure it out, they gave up/didn't report, or, we just have very few macOS users.
  >
  > Related to #18128.

  That's how it looks on macOS:

  ![image](https://github.com/bitcoin/bitcoin/assets/32963518/baa637bb-256b-4b24-8645-8c2754c2ae64)

ACKs for top commit:
  Sjors:
    tACK b5790c35f7
  jarolrod:
    ACK b5790c35f7
  TheCharlatan:
    utACK b5790c35f7

Tree-SHA512: 6e9cb3ab0f60f8a92bfec50577e8d096c5b23ec09ebbb334826415609140ddc96d470aea37379495c1c6bb1beec0d306b09460f62e1543bb0f4396c10a1dfbe2
2023-09-20 11:40:47 +00:00
fanquake
1bf915db99
Merge bitcoin/bitcoin#28470: fuzz: Rework addr fuzzing
fad52baf1e fuzz: Rework addr fuzzing (MarcoFalke)
fa5b6d29ee fuzz: Drop unused params from serialize helpers (MarcoFalke)

Pull request description:

  Some minor fixups to addr fuzzing

ACKs for top commit:
  dergoegge:
    utACK fad52baf1e

Tree-SHA512: 6a2b07fb1a65cf855d5e7c0a52bfcb81d46dbc5d4b3e72cef359987cbd28dbfeb2fc54f210e9737cb131b40ac5f88a90e9af284e441e0b37196121590bbaf015
2023-09-20 11:38:56 +00:00
Andrew Chow
abe4fedab7
Merge bitcoin/bitcoin#28125: wallet: bugfix, disallow migration of invalid scripts
8e7e3e6149 test: wallet, verify migration doesn't crash for an invalid script (furszy)
1de8a2372a wallet: disallow migration of invalid or not-watched scripts (furszy)

Pull request description:

  Fixing #28057.

  The legacy wallet allows to import any raw script (#28126), without
  checking if it was valid or not. Appending it to the watch-only set.

  This causes a crash in the migration process because we are only
  expecting to find valid scripts inside the legacy spkm.

  These stored scripts internally map to `ISMINE_NO` (same as if they
  weren't stored at all..).

  So we need to check for these special case, and take into account that
  the legacy spkm could be storing invalid not watched scripts.

  Which, in code words, means `IsMineInner()` returning
  `IsMineResult::INVALID` for them.

  Note:
  To verify this, can run the test commit on top of master.
  `wallet_migration.py` will crash without the bugfix commit.

ACKs for top commit:
  achow101:
    ACK 8e7e3e6149

Tree-SHA512: c2070e8ba78037a8f573b05bf6caa672803188f05429adf5b93f9fc1493faedadecdf018dee9ead27c656710558c849c5da8ca5f6f3bc9c23b3c4275d2fb50c7
2023-09-19 13:10:57 -04:00
stratospher
28bac81a34 test: add functional test for getaddrmaninfo 2023-09-19 22:38:56 +05:30
fanquake
53313c49d6
Merge bitcoin/bitcoin#28246: wallet: Use CTxDestination in CRecipient instead of just scriptPubKey
ad0c469d98 wallet: Use CTxDestination in CRecipient rather than scriptPubKey (Andrew Chow)
07d3bdf4eb Add PubKeyDestination for P2PK scripts (Andrew Chow)
1a98a51c66 Allow CNoDestination to represent a raw script (Andrew Chow)
8dd067088d Make WitnessUnknown members private (Andrew Chow)

Pull request description:

  For silent payments, we want to provide a `SilentPaymentsDestination` to be used as the recipient, which requires `CRecipient` to use something other than just the `scriptPubKey` as we cannot know the output script for a silent payment prior to transaction creation. `CTxDestination` seems like the obvious place to add a `SilentPaymentsDestination` as it is our internal representation of an address.

  In order to still allow paying to arbitrary scriptPubKeys (e.g. for data carrier outputs, or the user hand crafted a raw transaction that they have given to `fundrawtransaction`), `CNoDestination` is changed to contain raw scripts.

  Additionally, P2PK scripts are now interpreted as a new `PubKeyDestination` rather than `PKHash`. This results in some things that would have given an address for P2PK scripts to no longer do so. This is arguably more correct.

  `ExtractDestination`'s behavior is slightly changed for the above. It now returns `true` for those destinations that have addresses, so P2PK scripts now result in `false`. Even though it returns false for `CNoDestination`, the script will now be included in that `CNoDestination`.

  Builds on #28244

ACKs for top commit:
  josibake:
    ACK ad0c469d98

Tree-SHA512: ef3f8f3c7284779d9806c77c85b21caf910a79a1f7e7f1b51abcc0d7e074f14e00abf30f625a13075e41d94dad6202c10ddff462c0ee74c2ca4aab585b145a52
2023-09-19 16:48:43 +00:00
MarcoFalke
fa3b5e5e57
ci: Use nproc over MAKEJOBS in 01_base_install 2023-09-19 16:13:27 +00:00
MarcoFalke
fac29a0ab1
Remove SER_GETHASH, hard-code client version in CKeyPool serialize
It was never set, so it can be removed along with any code reading it.
2023-09-19 16:11:59 +00:00
fanquake
737aac8cc8
Merge bitcoin/bitcoin#28497: ci: Reintroduce fixed "test-each-commit" job
27b636a921 ci: Reintroduce fixed "test-each-commit" job (Hennadii Stepanov)

Pull request description:

  This is a fixed version of https://github.com/bitcoin/bitcoin/pull/28279:
  > Currently, if a pull request has more than one commit, previous commits may fail to compile, or may fail the tests. This is problematic, because it breaks git-bisect, or worse.
  >
  > Fix this by adding a CI task for this.

  The new job checks at most 6 commits of a pull request, excluding the top one.

  The maximum number of tested commits is 6, which derives from the time [constrains](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idtimeout-minutes).

  For historical context, please see:
  - https://github.com/bitcoin/bitcoin/pull/28279
  - https://github.com/bitcoin/bitcoin/pull/28477
  - https://github.com/bitcoin/bitcoin/pull/28478

  **A note for reviewers:** To test scripts locally, ensure that you works with a _shallow_ copy of the repo.

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 27b636a921

Tree-SHA512: 0c69ced13509fa0ed2dd6ef13f4c710d678e31b294b6318b59ab1ba899086a71b5c893aaf70e143036349329167bf8e16bdca319b2c761e2aef6222d0db1470c
2023-09-19 16:09:12 +00:00
fanquake
a5979a8d4a
Merge bitcoin/bitcoin#28506: fuzz: Add missing PROVIDE_FUZZ_MAIN_FUNCTION guard to __AFL_FUZZ_INIT
fa33b2c889 fuzz: Add missing PROVIDE_FUZZ_MAIN_FUNCTION guard to __AFL_FUZZ_INIT (MarcoFalke)

Pull request description:

  Should fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=62455

ACKs for top commit:
  dergoegge:
    utACK fa33b2c889

Tree-SHA512: 735926f7f94ad1c3c5dc0fc62a2ef3a85abae25f4fe1e7654c2857ce3e867667ed28da58ab36281d730d3e206a0728cb429671ea5d3ccd11519e637eb191f70d
2023-09-19 15:13:26 +00:00