Commit graph

35554 commits

Author SHA1 Message Date
Leonardo Araujo
2dede9f675 Adjust RPCTypeCheckObj error string 2022-10-10 18:08:00 -03:00
MacroFake
9ca39d69df
Merge bitcoin/bitcoin#26254: iwyu: Add zmq source files
13afcc0cd4 iwyu: Add zmq source files (Hennadii Stepanov)

Pull request description:

ACKs for top commit:
  fanquake:
    ACK 13afcc0cd4

Tree-SHA512: 7af95e991fc2782aeba2edfef0a2f75f9c361058295586adb062087aa31c47cfcce2425aee9dd5153e18e018cf1f9272c9617c671b7262db55f241526c3fcb15
2022-10-10 18:08:45 +02:00
Hennadii Stepanov
13afcc0cd4
iwyu: Add zmq source files 2022-10-10 15:44:02 +01:00
fanquake
57c192767b
Merge bitcoin/bitcoin#25322: build: Fix capnp package build for Android
8b8edc25c1 build: Specify native binaries explicitly when building `capnp` package (Hennadii Stepanov)
a413595c37 build: Fix `capnp` package build for Android (Hennadii Stepanov)

Pull request description:

  On master (e3c08eb620):
  ```
  $ make -C depends capnp MULTIPROCESS=1 HOST=aarch64-linux-android ANDROID_SDK=$ANDROID_HOME ANDROID_NDK=$ANDROID_HOME/ndk/23.2.8568313 ANDROID_API_LEVEL=28 ANDROID_TOOLCHAIN_BIN=$ANDROID_HOME/ndk/23.2.8568313/toolchains/llvm/prebuilt/linux-x86_64/bin
  ...
  ld: error: unable to find library -lkj
  ...
  ```

  This PR fixes this error, and also improves configuring according to the docs.

ACKs for top commit:
  ryanofsky:
    Code review ACK 8b8edc25c1. I'd be a little curious to know what causes the error and how `--disable-shared` fixes it, but these changes all look good

Tree-SHA512: 1b07b75f2a83932d8dc1f007e42a67d8327bd5fe4566f554dab4599e2a1e04b0144648790a1fd2ab1c295dba728586035aa0ebdbe5cf49df048ec87736895aaf
2022-10-10 21:04:32 +08:00
MacroFake
239757409b
Merge bitcoin/bitcoin#26118: log: Use steady clock for bench logging
fabf1cdb20 Use steady clock for bench logging (MacroFake)
faed342a23 scripted-diff: Rename time symbols (MacroFake)

Pull request description:

  Instead of using `0.001` and similar constants to "convert" an int64_t to milliseconds, use the type-safe `Ticks<>` helper. Also, use steady clock instead of system clock, since the durations are used for benchmarking.

ACKs for top commit:
  fanquake:
    ACK fabf1cdb20 - validation bench output still looks sane.

Tree-SHA512: e6525b5fdad6045ca500c56014897d7428ad288aaf375933d3b5939feddf257f6910d562eb66ebcde9186bef9a604ee8d763a318253838318d59df2a285be7c2
2022-10-10 12:00:34 +02:00
fanquake
866dd664a1
Merge bitcoin/bitcoin#26196: kernel: move RunCommandParseJSON to its own file
43b8777dc3 refactor: move run_command from util to common (Cory Fields)
192325a77d kernel: move RunCommandParseJSON to its own file (Cory Fields)

Pull request description:

  Because libbitcoinkernel does not include this new object, this has the side-effect of eliminating its unnecessary `boost::process` dependency.

  This leaves libbitcoinkernel with 3 remaining boost dependencies:
  - `boost::date_time` for `util/time.cpp`, which I'll separate out next. Exactly like this PR.
  - `boost::signals2` for which I have a POC re-implementation here: https://github.com/theuni/bitcoin/commits/replace-boost-signals
  - `boost::multi_index` which I'm not sure about yet.

ACKs for top commit:
  ryanofsky:
    Code review ACK 43b8777dc3. Could consider squashing the two commits, so the code just moves once instead of twice.
  fanquake:
    ACK 43b8777dc3

Tree-SHA512: f2a46cac34aaadfb8a1442316152ad354f6990021b82c78d80cae9fd43cd026209ffd62132eaa99d5d0f8cf34e996b6737d318a9d9a3f1d2ff8d17d697abf26d
2022-10-10 17:58:18 +08:00
fanquake
869342f7fa
Merge bitcoin/bitcoin#26282: wallet: have prune error take precedence over assumedvalid
1c36bafc5f wallet: have prune error take precedence over assumedvalid (James O'Beirne)

Pull request description:

  Fixes https://github.com/bitcoin/bitcoin/pull/23997#discussion_r891412739.

  From Russ Yanofsky:

  > Agree with all of Marco's points here and think this should be updated
  >
  > If havePrune and hasAssumedValidChain are both true, better to show havePrune error message.  Assumed-valid error message is vague and not very actionable.  Would suggest "Error loading wallet. Wallet requires blocks to be downloaded, and software does not currently support loading wallets while blocks are being downloaded out of order though assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height {block_height}"

ACKs for top commit:
  MarcoFalke:
    ACK 1c36bafc5f
  aureleoules:
    ACK 1c36bafc5f

Tree-SHA512: bfb0024bb962525cbbd392ade3c0331a8b0525e7f2f2ab52b2dbb9b6dd6311070d85ecb762a7689db84a30991971865698ab6fec187206e6a92133790c5a91dc
2022-10-10 17:04:43 +08:00
fanquake
9eaa5dbc81
Merge bitcoin/bitcoin#25073: test: Cleanup miner_tests
faa15527d7 test: Use dedicated mempool in TestBasicMining (MacroFake)
fafab384a0 test: Use dedicated mempool in TestPackageSelection (MacroFake)
fa4055d79c test: Use dedicated mempool in TestPrioritisedMining (MacroFake)
fa29218285 test: Pass mempool reference to AssemblerForTest (MacroFake)

Pull request description:

  This cleans up the miner tests:

  * Removes duplicate/redundant and thus confusing chainparams object.
  * Uses a fresh mempool for each subtest instead of using the "global" one from the testing setup. This makes it easier to follow the tests in smaller scopes. Also it makes sure the mempool is truly cleared by reconstructing it. Finally, this removes calls to `clear`, see https://github.com/bitcoin/bitcoin/pull/19909

ACKs for top commit:
  glozow:
    utACK faa15527d7

Tree-SHA512: ced1260f6ab70fba74b0fac7ff4fc7adfddcd2f3bee785249d2a4a9055ac253eff9090edbda7a17e72a71a81b56ff708d5ff64e1f57ebc7b7747d6c88fec51e3
2022-10-10 16:45:39 +08:00
MacroFake
9f44f2e32f
Merge bitcoin/bitcoin#26284: Fix comment typos
adb1714426 Fix comment typos in scriptpubkeyman.cpp, wallet.cpp, wallet.h (Dimitris Tsapakidis)

Pull request description:

  Fixes a number of comment typos found in the code.

Top commit has no ACKs.

Tree-SHA512: c2c996b66d33ecf0ee734b76303a0f2444e184d2f3ff6931768712ca51011ad51e54336c33a2ff55133766d20ae6adcbb14ddc754dde58b1fe9167d68f54fec5
2022-10-10 09:32:51 +02:00
fanquake
515c9c4c2e
Merge bitcoin/bitcoin#26183: build: split ARM crc & crypto extension checks
20adaeaef5 build: split ARM crc & crypto extension checks (fanquake)

Pull request description:

  We currently perform the same check twice, to put the same set of flags in two different variables. Split the checks so we test for the `crc` and `crypto` extensions independently.

  If we don't want to split, we should just delete the second `AX_CHECK_COMPILE_FLAG` check, and set `ARM_CRC_CXXFLAGS` & `ARM_SHANI_CXXFLAGS` at the same time.

  Guix Build:
  ```bash
  045392a6a4f538723b7759c67eeafd832735de7294b72b3a7f488d05a13711f7  guix-build-20adaeaef5fa/output/aarch64-linux-gnu/SHA256SUMS.part
  054fda86577d757788a1c87508268402535fcbe869240309a2c91997234389cf  guix-build-20adaeaef5fa/output/aarch64-linux-gnu/bitcoin-20adaeaef5fa-aarch64-linux-gnu-debug.tar.gz
  92dc2513b2b6d87c0869ae18493fd9d0e2690b5b02bfd4310d54f4d394cfccdf  guix-build-20adaeaef5fa/output/aarch64-linux-gnu/bitcoin-20adaeaef5fa-aarch64-linux-gnu.tar.gz
  2515cfc708cc6ce0e650ca00c49de8dad856b54741ddc0c195845fc6ce2d67db  guix-build-20adaeaef5fa/output/arm-linux-gnueabihf/SHA256SUMS.part
  fa0a956365e62b484f66dcf9763a02858db5c7e99317861819a54a15589ced80  guix-build-20adaeaef5fa/output/arm-linux-gnueabihf/bitcoin-20adaeaef5fa-arm-linux-gnueabihf-debug.tar.gz
  1b3ddf2b1bbdc7632696ca78908e69b4fd156ccf7afa8078b5541d2ac10ab931  guix-build-20adaeaef5fa/output/arm-linux-gnueabihf/bitcoin-20adaeaef5fa-arm-linux-gnueabihf.tar.gz
  f87d8e23df60b208a631f6642f6c2cc0fc8e4e5e9563b36d1de9d371f22a69d9  guix-build-20adaeaef5fa/output/arm64-apple-darwin/SHA256SUMS.part
  c24ac07bfa935fd40358823d95ef01128a03b80deec6b2cb8bed122994e8adc2  guix-build-20adaeaef5fa/output/arm64-apple-darwin/bitcoin-20adaeaef5fa-arm64-apple-darwin-unsigned.dmg
  696660e030accadc27901dfb4e120aa2fefefa8cc2a33ae887e3c98e5d4795f5  guix-build-20adaeaef5fa/output/arm64-apple-darwin/bitcoin-20adaeaef5fa-arm64-apple-darwin-unsigned.tar.gz
  30dcd3f543781ac0e07e36336c2901a25a0829e0d1425c25b3c7aba1d0e5420e  guix-build-20adaeaef5fa/output/arm64-apple-darwin/bitcoin-20adaeaef5fa-arm64-apple-darwin.tar.gz
  4d63db45f28fcb99aa8f3b30cf06afef80dd308a8d2fdf874752accb3f341258  guix-build-20adaeaef5fa/output/dist-archive/bitcoin-20adaeaef5fa.tar.gz
  eb208b98b3118e9f8240aab91c7ecb2f9b778109bc19d81d0ba73b3e35aa1123  guix-build-20adaeaef5fa/output/powerpc64-linux-gnu/SHA256SUMS.part
  8b0de7008b1932ed18d3ab71ca309dc4919096e226e0a7197bd192e1ba96da82  guix-build-20adaeaef5fa/output/powerpc64-linux-gnu/bitcoin-20adaeaef5fa-powerpc64-linux-gnu-debug.tar.gz
  bcbc269cc4b5883397c516ef3ef6df564f4a81c240d5afcf912a2bf9554ff148  guix-build-20adaeaef5fa/output/powerpc64-linux-gnu/bitcoin-20adaeaef5fa-powerpc64-linux-gnu.tar.gz
  e5f7fd823056449a495a68d18fe941b472479bc59d9d4d11a041a4e2cc4044ec  guix-build-20adaeaef5fa/output/powerpc64le-linux-gnu/SHA256SUMS.part
  73ee7e786372b32ab840f0c00ca0479ddd022b3d37219cd929cb49f744c174e3  guix-build-20adaeaef5fa/output/powerpc64le-linux-gnu/bitcoin-20adaeaef5fa-powerpc64le-linux-gnu-debug.tar.gz
  08f64c9aae4d9beef88d8fbae8ad0152517de74bedc88540775c4f757c8b6b9a  guix-build-20adaeaef5fa/output/powerpc64le-linux-gnu/bitcoin-20adaeaef5fa-powerpc64le-linux-gnu.tar.gz
  fe3c28fdb1ee9d5b6ca3ba4510d61c052567edb3b93fdde929ed197072c0fd66  guix-build-20adaeaef5fa/output/riscv64-linux-gnu/SHA256SUMS.part
  890d6b96edcc431620eede6239dec51368aff917010e03dabeb29d6a672d7a28  guix-build-20adaeaef5fa/output/riscv64-linux-gnu/bitcoin-20adaeaef5fa-riscv64-linux-gnu-debug.tar.gz
  df1fc0c9af4799cfe170444e21965f2a600aa193fdd0da542fedceeb3081b194  guix-build-20adaeaef5fa/output/riscv64-linux-gnu/bitcoin-20adaeaef5fa-riscv64-linux-gnu.tar.gz
  f69cae0b2d0eadb336cf314a888b1e0bed241f38954fe58ca9c9c2d00e49b74e  guix-build-20adaeaef5fa/output/x86_64-apple-darwin/SHA256SUMS.part
  acc5fa9725bba738d10bb8b1e7df2d8a7b0e648015e1c046f67451d343f68224  guix-build-20adaeaef5fa/output/x86_64-apple-darwin/bitcoin-20adaeaef5fa-x86_64-apple-darwin-unsigned.dmg
  7e4d8cb6d74434ba9084f487187c49cd5a4138c9ae03a6c2236cdffadb236bc8  guix-build-20adaeaef5fa/output/x86_64-apple-darwin/bitcoin-20adaeaef5fa-x86_64-apple-darwin-unsigned.tar.gz
  8d93add28b20dfb2a556d3867cfbf218db336d7eefee6ab6f76a1bb4dd4ae20b  guix-build-20adaeaef5fa/output/x86_64-apple-darwin/bitcoin-20adaeaef5fa-x86_64-apple-darwin.tar.gz
  ba0863eda963db706d2880daa8bc526e6332097010fa7227f513a2d715b6cd6c  guix-build-20adaeaef5fa/output/x86_64-linux-gnu/SHA256SUMS.part
  6915794f3cdc8ad9b305b6baa58f89f7493097b88c0af190d52d93457a17e8d8  guix-build-20adaeaef5fa/output/x86_64-linux-gnu/bitcoin-20adaeaef5fa-x86_64-linux-gnu-debug.tar.gz
  467b05298058ec507c3b247c423f3ea7e027ecf62e45d7ae4b81160118bc0d02  guix-build-20adaeaef5fa/output/x86_64-linux-gnu/bitcoin-20adaeaef5fa-x86_64-linux-gnu.tar.gz
  51a534803deaabcbba27d82359ef46e4d5b9e7b121ab71e1975c2a0d1c4c6f45  guix-build-20adaeaef5fa/output/x86_64-w64-mingw32/SHA256SUMS.part
  c9c5496f20bac01ed6439746aff9ca3dd55708718902c898e99f3d5741b167a3  guix-build-20adaeaef5fa/output/x86_64-w64-mingw32/bitcoin-20adaeaef5fa-win64-debug.zip
  cfaac54be36789927f83172c0af44c50648f63df7cdc9d81774a170e5ab6e3e5  guix-build-20adaeaef5fa/output/x86_64-w64-mingw32/bitcoin-20adaeaef5fa-win64-setup-unsigned.exe
  759b79660c291dcc7da88088de3bb666162fed5c9d94bb24f10cef6e781c565f  guix-build-20adaeaef5fa/output/x86_64-w64-mingw32/bitcoin-20adaeaef5fa-win64-unsigned.tar.gz
  f0124333d384ff6962e2131c7b2814bf5c968e77b63ff1b2c7d19cb4c571757c  guix-build-20adaeaef5fa/output/x86_64-w64-mingw32/bitcoin-20adaeaef5fa-win64.zip
  ```

ACKs for top commit:
  jarolrod:
    ACK 20adaeaef5

Tree-SHA512: 8b515b95ba4d41ca2ce91448339841dcfb80feb028e9e3bc67a72e72d93669e1257534c11286489a60ae240f6ad6e68f56615818fefd1c09a07a1bee4976fa6e
2022-10-10 15:30:51 +08:00
fanquake
857f07dfd0
Merge bitcoin/bitcoin#26277: test: Remove confusing DUMMY_P2WPKH_SCRIPT
fa8a305ddd test: Remove confusing DUMMY_P2WPKH_SCRIPT (MacroFake)

Pull request description:

  It is confusing because, it is *not* a P2WPKH script, and it is nonstandard.

  See also https://github.com/bitcoin/bitcoin/pull/26265/files#r989827855

  Fix all issues by removing it, and also remove the no longer needed `-acceptnonstdtxn` setting from the test.

ACKs for top commit:
  instagibbs:
    ACK fa8a305ddd
  theStack:
    Code-review ACK fa8a305ddd 📜

Tree-SHA512: 64f3e0009b055e4fd4428b20f3e85582e1608e9b06e500b8fbfeb91fc35ce510e69d051e8f48ce35d0320067793e12f4423b214cc1f68c217a5872e0ad97d211
2022-10-10 15:28:48 +08:00
fanquake
4175c332b9
Merge bitcoin/bitcoin#26215: index: Improve BaseIndex::BlockUntilSyncedToCurrentChain reliability
8891949bdc index: Improve BaseIndex::BlockUntilSyncedToCurrentChain reliability (Ryan Ofsky)

Pull request description:

  Since commit f08c9fb0c6 from PR https://github.com/bitcoin/bitcoin/pull/21726, index  `BlockUntilSyncedToCurrentChain` behavior has been less reliable, and there has also been a race condition in the `coinstatsindex_initial_sync` unit test.

  It seems better for `BlockUntilSyncedToCurrentChain` to actually wait for the last connected block to be fully processed, than to be able to return before prune locks are set, so this switches the order of `m_best_block_index = block;` and `UpdatePruneLock` statements in `SetBestBlockIndex` to make it more reliable.

  Also since commit f08c9fb0c6, there has been a race condition in the `coinstatsindex_initial_sync` test. Before that commit, the atomic index best block pointer `m_best_block_index` was updated as the last step of `BaseIndex::BlockConnected`, so `BlockUntilSyncedToCurrentChain` could safely be used in tests to wait for the last `BlockConnected` notification to be finished before stopping and destroying the index.  But after that commit, calling `BlockUntilSyncedToCurrentChain` is no longer sufficient, and there is a race between the test shutdown code which destroys the index object and the new code introduced in that commit calling `AllowPrune()` and `GetName()` on the index object. Reproducibility instructions for this are in https://github.com/bitcoin/bitcoin/issues/25365#issuecomment-1259744133

  This commit fixes the `coinstatsindex_initial_sync` race condition, even though it will require an additional change to silence TSAN false positives, https://github.com/bitcoin/bitcoin/pull/26188, after it is fixed. So this partially addresses but does not resolve the bug reporting TSAN errors https://github.com/bitcoin/bitcoin/issues/25365.

  There is no known race condition outside of test code currently, because the bitcoind `Shutdown` function calls `FlushBackgroundCallbacks` not `BlockUntilSyncedToCurrentChain` to safely shut down.

  Co-authored-by: vasild
  Co-authored-by: MarcoFalke

ACKs for top commit:
  mzumsande:
    re-ACK 8891949bdc

Tree-SHA512: 52e29e3772a0c92873c54e5ffb31dd66a909b68a2031b7585713cd1d976811289c98bd9bb41679a8689062f03be4f97bb8368696e789caa4607c2fd8b1fe289b
2022-10-10 14:23:00 +08:00
fanquake
cf3db7c256
Merge bitcoin/bitcoin#26258: refactor: Remove unused CDataStream::rdbuf method
fabbbe32ee Remove unused CDataStream::rdbuf method (MacroFake)

Pull request description:

  It is unused and seems unlikely to be ever used.

ACKs for top commit:
  theStack:
    Code-review ACK fabbbe32ee
  aureleoules:
    ACK fabbbe32ee

Tree-SHA512: 5804642658f96a0fb51482ebf3a062bb0f997c1e0527455afa4aceeeb6c1ad139a98b14a7c8a0909daba733a83bdc24fcadad45060ead4be6eb3dc3e66c129e2
2022-10-10 14:14:15 +08:00
glozow
d33c5894e9
Merge bitcoin/bitcoin#26103: refactor: mempool: use CTxMemPool::Limits
33b12e5df6 docs: improve docs where MemPoolLimits is used (stickies-v)
6945853c0b test: use NoLimits() in MempoolIndexingTest (stickies-v)
3a86f24a4c refactor: mempool: use CTxMempool::Limits (stickies-v)
b85af25f87 refactor: mempool: add MemPoolLimits::NoLimits() (stickies-v)

Pull request description:

  Mempool currently considers 4 limits regarding ancestor and descendant count and size, which get passed around between functions quite a bit. This PR uses `CTxMemPool::Limits` introduced in https://github.com/bitcoin/bitcoin/pull/25290 to simplify those signatures and callsites.

  The purpose of this PR is to improve readability and maintenance, without behaviour change.

  As noted in the first commit "refactor: mempool: change MemPoolLimits members to uint", we currently have an underflow issue where a user could pass a negative `-limitancestorsize`, which is eventually cast to an unsigned integer. This behaviour already exists. Because it's orthogonal and to minimize scope, I think this should be fixed in a separate PR.

ACKs for top commit:
  hebasto:
    ACK 33b12e5df6, I have reviewed the code and it looks OK, I agree it can be merged.
  glozow:
    reACK 33b12e5df6

Tree-SHA512: 591c6dcee1894f1c3ca28b34a680eeadcf0d40cda92451b4a422c03087b27d682b5e30ba4367abd75a99b5ccb115b7884b0026958d3c7dddab030549db5a4056
2022-10-09 10:28:32 -04:00
Dimitris Tsapakidis
adb1714426 Fix comment typos in scriptpubkeyman.cpp, wallet.cpp, wallet.h
Fix comment typos:
sigature -> signature
ponter -> pointer
it's key -> its key
2022-10-09 17:19:59 +03:00
glozow
ec8016eba7
Merge bitcoin/bitcoin#26281: docs: fix m_children to be a member of CTxMemPoolEntry
01bf4af4f2 docs: fix m_children to be a member of CTxMemPoolEntry (stickies-v)

Pull request description:

  Small documentation fix to reflect that `m_children` [is a member](73b61717a9/src/txmempool.h (L99)) of `CTxMemPoolEntry`, not `CTxMemPool`

ACKs for top commit:
  hebasto:
    ACK 01bf4af4f2, wrong wording was introduced in bitcoin/bitcoin#19478.
  glozow:
    ACK 01bf4af4f2

Tree-SHA512: b66c43b92fda44682b1f67c43073ca9e133a6dc03cd28253e571e67170531138c20b22ffdb08f312fb2d47a1f869b876611646b54325c8b614d12049befad578
2022-10-09 10:17:02 -04:00
James O'Beirne
1c36bafc5f
wallet: have prune error take precedence over assumedvalid
From Russ Yanofsky:

"Agree with all of Marco's points here and think this should be updated

If havePrune and hasAssumedValidChain are both true, better to show
havePrune error message.  Assumed-valid error message is vague and not
very actionable.  Would suggest "Error loading wallet. Wallet requires
blocks to be downloaded, and software does not currently support loading
wallets while blocks are being downloaded out of order though assumeutxo
snapshots. Wallet should be able to load successfully after node sync
reaches height {block_height}"

Co-authored-by: MacroFake <MarcoFalke@gmail.com>
Co-authored-by: Russ Yanofsky <russ@yanofsky.org>
2022-10-07 15:19:31 -04:00
stickies-v
01bf4af4f2
docs: fix m_children to be a member of CTxMemPoolEntry 2022-10-07 15:06:45 +01:00
MacroFake
fa8a305ddd
test: Remove confusing DUMMY_P2WPKH_SCRIPT 2022-10-07 13:11:05 +02:00
MacroFake
73b61717a9
Merge bitcoin/bitcoin#26272: test: Prevent UB in minisketch_tests.cpp
97007e2b9b test: Prevent UB in `minisketch_tests.cpp` (Hennadii Stepanov)

Pull request description:

  [`std::optional::operator*`](https://en.cppreference.com/w/cpp/utility/optional/operator*), which follows after the changed line, can cause UB.

  This PR addresses https://github.com/bitcoin/bitcoin/issues/26262#issuecomment-1268855418

ACKs for top commit:
  stickies-v:
    ACK 97007e2b9b

Tree-SHA512: a7dde8dac0cbdfa362fa1158b4564eccff9405852612227d581690c9a34084b3467ae6d4c0269262688d75339dcea90aaa38fccbba9be92d2643c2113860f3d6
2022-10-06 16:01:17 +02:00
Hennadii Stepanov
97007e2b9b
test: Prevent UB in minisketch_tests.cpp 2022-10-06 12:50:54 +01:00
glozow
292f652d53
Merge bitcoin/bitcoin#24364: refactor: remove duplicate code from BlockAssembler
0f40d65321 refactor: remove duplicate code from BlockAssembler (James O'Beirne)

Pull request description:

  Found while reminding myself how transactions are chosen for blocks. Take it or leave it!

ACKs for top commit:
  glozow:
    ACK 0f40d65321
  theStack:
    Concept and code-review ACK 0f40d65321

Tree-SHA512: 8a2694e670ce3fe897ab8f64f64c8df5f8487fc1264527a3abbcba0e5b921fb693416497ccd62508295bc33f202c65556b91b6af463acb91aab43138d2492c14
2022-10-06 12:50:33 +01:00
Ryan Ofsky
8891949bdc index: Improve BaseIndex::BlockUntilSyncedToCurrentChain reliability
Since commit f08c9fb0c6 from PR
https://github.com/bitcoin/bitcoin/pull/21726, index
`BlockUntilSyncedToCurrentChain` behavior has been less reliable, and there has
also been a race condition in the `coinstatsindex_initial_sync` unit test.

It seems better for `BlockUntilSyncedToCurrentChain` to actually wait for the
last connected block to be fully processed, than to be able to return before
prune locks are set, so this switches the order of `m_best_block_index =
block;` and `UpdatePruneLock` statements in `SetBestBlockIndex` to make it more
reliable.

Also since commit f08c9fb0c6, there has been a
race condition in the `coinstatsindex_initial_sync` test. Before that commit,
the atomic index best block pointer `m_best_block_index` was updated as the
last step of `BaseIndex::BlockConnected`, so `BlockUntilSyncedToCurrentChain`
could safely be used in tests to wait for the last `BlockConnected`
notification to be finished before stopping and destroying the index. But
after that commit, calling `BlockUntilSyncedToCurrentChain` is no longer
sufficient, and there is a race between the test shutdown code which destroys
the index object and the new code introduced in that commit calling
`AllowPrune()` and `GetName()` on the index object. Reproducibility
instructions for this are in
https://github.com/bitcoin/bitcoin/issues/25365#issuecomment-1259744133

This commit fixes the `coinstatsindex_initial_sync` race condition, even though
it will require an additional change to silence TSAN false positives,
https://github.com/bitcoin/bitcoin/pull/26188, after it is fixed. So this
partially addresses but does not resolve the bug reporting TSAN errors
https://github.com/bitcoin/bitcoin/issues/25365.

There is no known race condition outside of test code currently, because the
bitcoind `Shutdown` function calls `FlushBackgroundCallbacks` not
`BlockUntilSyncedToCurrentChain` to safely shut down.

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Co-authored-by: MacroFake <falke.marco@gmail.com>
2022-10-05 11:06:58 -04:00
MacroFake
5e82b9ba96
Merge bitcoin/bitcoin#26252: refactor: Make 64-bit shift explicit
5c5b85d0e7 refactor: Make 64-bit shift explicit (Hennadii Stepanov)

Pull request description:

  [`std::array::at()`](https://en.cppreference.com/w/cpp/container/array/at) expects an argument of the `size_t` type. This PR avoids implicit type conversion (for both 64-bit and 32-bit systems).

  Also it enables MSVC warning [C4334](https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4334) for all codebase.

ACKs for top commit:
  MarcoFalke:
    ACK 5c5b85d0e7 🚎
  jonatack:
    Code review ACK 5c5b85d0e7

Tree-SHA512: fda850a42068f2ada9f877fac9ff8af1e22b5dcb3e708f5b95c316e77c52c72d33cd9ec6507a7f5d1731d1afdf5af6dc65025d388cc480f82c46f4d88ef2d306
2022-10-05 15:46:23 +02:00
MacroFake
fabbbe32ee
Remove unused CDataStream::rdbuf method
It is unused and seems unlikely to be ever used.
2022-10-05 15:29:36 +02:00
stickies-v
33b12e5df6
docs: improve docs where MemPoolLimits is used 2022-10-05 13:09:08 +01:00
stickies-v
6945853c0b
test: use NoLimits() in MempoolIndexingTest
The (100, 1000000, 1000, 1000000) limits are arbitrarily high and
don't restrict anything, they are just meant to calculate ancestors
properly. Using NoLimits() makes this intent more clear and simplifies
the code.
2022-10-05 13:07:11 +01:00
stickies-v
3a86f24a4c
refactor: mempool: use CTxMempool::Limits
Simplifies function signatures by removing repetition of all the
ancestor/descendant limits,  and increases readability by being
more verbose by naming the limits, while still reducing the LoC.
2022-10-05 13:07:11 +01:00
stickies-v
b85af25f87
refactor: mempool: add MemPoolLimits::NoLimits()
There are quite a few places in the codebase that require us to
construct a CTxMemPool without limits on ancestors and descendants.
This helper function allows us to get rid of all that duplication.
2022-10-05 13:07:11 +01:00
MacroFake
faa15527d7
test: Use dedicated mempool in TestBasicMining
No need for a shared mempool. Also remove unused chainparams parameter.

Can be reviewed with --ignore-all-space
2022-10-05 13:36:57 +02:00
MacroFake
fafab384a0
test: Use dedicated mempool in TestPackageSelection
No need for a shared mempool. Also remove unused chainparams parameter.
2022-10-05 13:36:56 +02:00
MacroFake
fa4055d79c
test: Use dedicated mempool in TestPrioritisedMining
No need for a shared mempool. Also remove unused chainparams parameter.
2022-10-05 13:35:18 +02:00
MacroFake
fa29218285
test: Pass mempool reference to AssemblerForTest 2022-10-05 13:34:36 +02:00
MacroFake
b2e6d37b51
Merge bitcoin/bitcoin#26256: ci: Remove clang-format from lint task
fa04376554 Remove clang-format from lint task (MacroFake)

Pull request description:

  clang-format could be used in scripted diffs, but remained largely unused.

  So remove the install bloat, as it is unlikely to be used in the future.

ACKs for top commit:
  fanquake:
    ACK fa04376554
  hebasto:
    ACK fa04376554

Tree-SHA512: e0a3ee47d2aa2565dd34676914c558c985eaeb522a05f10bcaac115871edcf0d7f101b517e4d452ca5223c40b18ad02883c31e2da3d1f4ff86464a9af0097b11
2022-10-05 11:52:46 +02:00
MacroFake
fa04376554
Remove clang-format from lint task
clang-format could be used in scripted diffs, but remained largely
unused.
2022-10-05 10:52:42 +02:00
MacroFake
d3cdd37d92
Merge bitcoin/bitcoin#26250: fuzz: add mempool_utils.cpp
8a6b6dfcd8 fuzz: pass max fee into ConsumeTxMemPoolEntry (fanquake)
eb15569280 fuzz: add util/mempool/h.cpp (fanquake)

Pull request description:

  Moving the heavy (Boost) mempool code out of fuzz/util.h. Means that (for ex) a crypto_common fuzz unit doesn't need to care about seeing endless Boost headers. This results in a ~10% speedup (for me) when compiling the fuzz tests. Your results may vary.

ACKs for top commit:
  MarcoFalke:
    review ACK 8a6b6dfcd8 🍮

Tree-SHA512: 27dc9d9581ac0b1b319cc0dc08fe5f8fbf9269386a5cb23f6fd5d8231bf015ed942ab4414d8001220541be0013756354578ddab1fec607c6fba04daf421bc870
2022-10-05 10:29:00 +02:00
Cory Fields
43b8777dc3 refactor: move run_command from util to common
Quoting ryanofsky: "util can be the library for things included in the kernel
which the kernel can depend on, and common can be the library for other code
that needs to be shared internally, but should not be part of the kernel or
shared externally."
2022-10-04 21:21:05 +00:00
Hennadii Stepanov
5c5b85d0e7
refactor: Make 64-bit shift explicit
Also this change enables MSVC warning C4334 for all codebase.

https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4334
2022-10-04 21:49:07 +01:00
fanquake
a23f8c8978
Merge bitcoin/bitcoin#26234: ci: Allow PIP_PACKAGES on centos
fa6054e952 ci: Allow PIP_PACKAGES on centos (MacroFake)
fac085a05c ci: Remove unused package (MacroFake)

Pull request description:

  This was added in 7fc5e865b9 but I can't see a reason why this should be forbidden.

  This is also needed for other changes (bumping the minimum python version).

ACKs for top commit:
  hebasto:
    re-ACK fa6054e952

Tree-SHA512: e8ead9ee00079024eb1e8c6e7b31c78cf2a3392159b444765c2ea9a58bed2a7550bf71083210692a45bb8ed7896cb882b72bf70baa13a2384864b2b510b73005
2022-10-04 21:39:29 +01:00
fanquake
b6565b65aa
Merge bitcoin/bitcoin#26243: test: Remove unused fCheckpointsEnabled from miner_tests
fa9436e908 test: Remove unused fCheckpointsEnabled from miner_tests (MacroFake)

Pull request description:

  The earliest checkpoint is at height 11111, so this can't possibly have any impact on this test.

ACKs for top commit:
  fanquake:
    ACK fa9436e908 - given the low number of blocks, having the additional check in `ContextualCheckBlockHeader()` enabled should be a no-op, so disabling and re-enabling is dead code.

Tree-SHA512: 7d1b952c297c915e9588761f82f5006cf5186b7ff30e8a1c702302e0b44afe536bde9eda8acf2995825ae01d2ad9d2393ae2feefb29f15676aaf71881941579b
2022-10-04 21:17:07 +01:00
fanquake
8a6b6dfcd8
fuzz: pass max fee into ConsumeTxMemPoolEntry 2022-10-04 21:12:50 +01:00
fanquake
eb15569280
fuzz: add util/mempool/h.cpp
Moving the mempool code (Boost) out of util.h, results in a ~10% speedup
(for me) when compiling the fuzz tests.
2022-10-04 21:12:50 +01:00
fanquake
d919e8d574
Merge bitcoin/bitcoin#26244: build, msvc: Enable C4834 warning
f3e40c481a build, msvc: Enable C4834 warning (Hennadii Stepanov)

Pull request description:

  Since bitcoin/bitcoin#26189 our codebase is  C4834 warning free.

  See https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/c4834.

ACKs for top commit:
  sipsorcery:
    tACK f3e40c481a.

Tree-SHA512: 2dd8fdb5d6c0d4eaf7ffec82039411963010ecea559adfe7e2b5587f467135bd665e0cef5c748e4d6d9ba3c2038d6a5c79980972884adbaaac3bda3b2fbbc408
2022-10-04 21:09:58 +01:00
MacroFake
2d8d9aecac
Merge bitcoin/bitcoin#26249: ci: Workaround Windows filesystem executable bit loss
b8d361ab6f ci: Workaround Windows filesystem executable bit loss (Hennadii Stepanov)

Pull request description:

  Fixes https://cirrus-ci.com/task/5946581265416192:
  ```
  From https://github.com/bitcoin/bitcoin
   * branch                refs/pull/26103/merge -> FETCH_HEAD
  error: Your local changes to the following files would be overwritten by checkout:
  ci/lint/04_install.sh
  ci/lint/06_script.sh
  contrib/devtools/gen-manpages.py
  contrib/devtools/symbol-check.py
  contrib/signet/getcoins.py
  contrib/signet/miner
  test/functional/feature_proxy.py
  test/functional/feature_taproot.py
  test/functional/interface_rest.py
  test/functional/mining_prioritisetransaction.py
  test/functional/rpc_blockchain.py
  test/functional/rpc_fundrawtransaction.py
  test/functional/rpc_help.py
  test/functional/rpc_rawtransaction.py
  test/functional/test_framework/test_framework.py
  test/functional/test_runner.py
  test/functional/wallet_basic.py
  test/functional/wallet_bumpfee.py
  test/functional/wallet_hd.py
  test/functional/wallet_importmulti.py
  test/functional/wallet_listdescriptors.py
  test/functional/wallet_multiwallet.py
  test/functional/wallet_resendwallettransactions.py
  test/functional/wallet_sendall.py
  test/lint/lint-locale-dependence.py
  Please commit your changes or stash them before you switch branches.
  Aborting
  ```

Top commit has no ACKs.

Tree-SHA512: 2b33d882a515bb17c7c2ae8cfe73541483cdc15e736909afaf42befc8f648dba5dc83ff58ebd6d38a5650a8eca01907ae6c61537927ac9718bd9582d8501647d
2022-10-04 16:40:08 +02:00
Hennadii Stepanov
b8d361ab6f
ci: Workaround Windows filesystem executable bit loss 2022-10-04 15:20:32 +01:00
Cory Fields
192325a77d kernel: move RunCommandParseJSON to its own file
Because libbitcoinkernel does not include this new object, this has the
side-effect of eliminating the unnecessary boost::process dependency.
2022-10-04 13:51:40 +00:00
fanquake
4fa50dd055
Merge bitcoin/bitcoin#26236: ci: Use same merge_script implementation for Windows as for all
37cf472063 ci: Use same `merge_script` implementation for Windows as for all (Hennadii Stepanov)
ac1d99240a ci: Move `git config` commands into script where they are used (Hennadii Stepanov)

Pull request description:

  This PR is a continuation of bitcoin/bitcoin#26202 and it suggests the same approach for the "Win64 native" CI task.

Top commit has no ACKs.

Tree-SHA512: 3154c9f30bc62549d738dc337e24f66614420417c349770c8381cc29f825f75695c9abbbb8dc57abbfda1375bf353f7c68e1a3766fd6d2440792e2d7fb68e15e
2022-10-04 13:45:40 +01:00
fanquake
92e9fc6d0d
Merge bitcoin/bitcoin#26128: doc: add missing historical release notes
cb075d245e doc: add historical 0.21.2 release notes (fanquake)
699f3429c6 doc: add historical 0.20.2 release notes (fanquake)

Pull request description:

  0.20.2 and 0.21.2 are missing from master.

ACKs for top commit:
  jarolrod:
    ACK cb075d245e

Tree-SHA512: f05fb2e5b589cd60581e724182c6f32f992a85e6dc41f5a91d5c6941869ff4c0a7f28a405b3dfc71d9660c1385a1a13d220aa50b2d69c7787cb7974a3e4bf814
2022-10-04 12:51:50 +01:00
Hennadii Stepanov
f3e40c481a
build, msvc: Enable C4834 warning
https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/c4834
2022-10-04 12:03:26 +01:00
MacroFake
fa9436e908
test: Remove unused fCheckpointsEnabled from miner_tests
The earliest checkpoint is at height 11111, so this can't possibly have
any impact on this test.
2022-10-04 12:40:19 +02:00