The boolean indicates whether the transaction was added without enforcing mempool
fee limits. m_mempool_limit_bypassed is the correct variable name.
Also changes NewMempoolTransactionInfo booleans descriptions to the format that
is consistent with the codebase.
11d797e3a078b8f5f0039a1073047d3f0a8c6cdc depends: Build `native_capnp` package with CMake (Hennadii Stepanov)
90389c95e9edf3d705fb9376388c83f07d1a570e depends: Build `capnp` package with CMake (Hennadii Stepanov)
Pull request description:
The first commit fixes two bugs when cross-compiling the `capnp` package on the master branch @ 160d23677ad799cf9b493eaa923b2ac080c3fb8e:
- for `x86_64-w64-mingw32` (see https://github.com/bitcoin/bitcoin/pull/28735#issuecomment-1790406668):
```
libtool: link: x86_64-w64-mingw32-g++-posix -shared -nostdlib /usr/lib/gcc/x86_64-w64-mingw32/12-posix/../../../../x86_64-w64-mingw32/lib/dllcrt2.o /usr/lib/gcc/x86_64-w64-mingw32/12-posix/crtbegin.o src/kj/.libs/cidr.o src/kj/.libs/common.o src/kj/.libs/units.o src/kj/.libs/memory.o src/kj/.libs/refcount.o src/kj/.libs/array.o src/kj/.libs/list.o src/kj/.libs/string.o src/kj/.libs/string-tree.o src/kj/.libs/source-location.o src/kj/.libs/hash.o src/kj/.libs/table.o src/kj/.libs/encoding.o src/kj/.libs/exception.o src/kj/.libs/debug.o src/kj/.libs/arena.o src/kj/.libs/io.o src/kj/.libs/mutex.o src/kj/.libs/thread.o src/kj/.libs/time.o src/kj/.libs/filesystem.o src/kj/.libs/filesystem-disk-unix.o src/kj/.libs/filesystem-disk-win32.o src/kj/.libs/test-helpers.o src/kj/.libs/main.o src/kj/parse/.libs/char.o -L/home/hebasto/git/bitcoin/depends/x86_64-w64-mingw32/lib -L/usr/lib/gcc/x86_64-w64-mingw32/12-posix -L/usr/lib/gcc/x86_64-w64-mingw32/12-posix/../../../../x86_64-w64-mingw32/lib -lstdc++ -lmingw32 -lgcc_s -lgcc -lmoldname -lmingwex -lmsvcrt -lkernel32 -lpthread -ladvapi32 -lshell32 -luser32 -lkernel32 -lmingw32 -lgcc_s -lgcc -lmoldname -lmingwex -lmsvcrt -lkernel32 /usr/lib/gcc/x86_64-w64-mingw32/12-posix/crtend.o -mthreads -O2 -mthreads -mthreads -o .libs/libkj-1-0-1.dll -Wl,--enable-auto-image-base -Xlinker --out-implib -Xlinker .libs/libkj.dll.a
/usr/bin/x86_64-w64-mingw32-ld: src/kj/.libs/cidr.o:cidr.c++:(.text+0x1dc): undefined reference to `__imp_inet_ntop'
/usr/bin/x86_64-w64-mingw32-ld: src/kj/.libs/cidr.o:cidr.c++:(.text+0x44b): undefined reference to `__imp_inet_pton'
collect2: error: ld returned 1 exit status
```
- for `arm64-apple-darwin`:
```
checking build system type... x86_64-pc-linux-gnu
checking host system type... Invalid configuration `arm64-apple-darwin': machine `arm64-apple' not recognized
configure: error: /bin/bash build-aux/config.sub arm64-apple-darwin failed
```
The second commit applies the same changes for the `native_capnp` package for [consistency](https://github.com/bitcoin/bitcoin/pull/28856#issuecomment-1807936546).
ACKs for top commit:
ryanofsky:
Code review ACK 11d797e3a078b8f5f0039a1073047d3f0a8c6cdc. Since last review arm64-apple-darwin platform is now mentioned in the commit message, and the change to `depends/packages/libmultiprocess.mk` in d1604d4b1d1ee8df279a1776303e167cc3d06193 which was unrelated (but probably still a good optimization) was reverted.
Tree-SHA512: c636e53073ce6fcda9724723bc59f3990fa9629a3b2f73d93dbc102a5a1badfbe8f4c5fef841f03588ebcad5cd4883f3ce32b128afcd75f6bc21eb801796a586
38816ff64ed90a55e4879e9b440cdc876302f750 fuzz: txorphan check wtxids using GenTxid::Wtxid not GenTxid::Txid (Greg Sanders)
Pull request description:
Fixes the bugs in the fuzz test with no more changes as an alternative to https://github.com/bitcoin/bitcoin/pull/28658
ACKs for top commit:
naumenkogs:
ACK 38816ff64ed90a55e4879e9b440cdc876302f750
dergoegge:
ACK 38816ff64ed90a55e4879e9b440cdc876302f750
Tree-SHA512: 5e46a83f2b2a2ac0672a63eb6200b019e01089ab1aa80c4ab869b6fcf27ccf2e84a064e96397f1a1869ccfa43b0c9638cbae681a27c4ca3c96ac71f41262601e
11b7269d83a56f919f9dddb7f7c72a96d428627f script: Enhance validations in utxo_snapshot.sh (pablomartin4btc)
Pull request description:
This PR resolves#27841 and some more:
- Ensure that the snapshot height is higher than the pruned block height when the node is pruned (Suggested by @Sjors [here](https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-1804941396)).
- Validate the correctness of the file path and check if the file already exists (@hazeycode's [#27845](https://github.com/bitcoin/bitcoin/pull/27845)).
- Make network activity disablement optional for the user (Suggested by @Sjors [here](https://github.com/bitcoin/bitcoin/pull/16899#discussion_r342735815) and [here](https://github.com/bitcoin/bitcoin/pull/16899#issuecomment-536520911)).
- Ensure the `reconsiderblock` command is triggered on exit (@hazeycode's same PR as above), even in the case of user interruption (Ctrl-C).
In order to perform some testing please follow the instructions in the description of previous @hazeycode's PR #27845.
ACKs for top commit:
Sjors:
tACK 11b7269d83a56f919f9dddb7f7c72a96d428627f
ryanofsky:
Code review ACK 11b7269d83a56f919f9dddb7f7c72a96d428627f
Tree-SHA512: 2b699894c6f732ad5104f5a2bcf5dc86ed31edcc9d664690cab55b94a8ab00e2ca5bde901ee1d63acddca7ea80ad1734d8cfe78f9c02f8470f264fe93a2af759
8f6ab318635d823a7e6ad9020ac17b8d2dd0e91b init: don't delete PID file if it was not generated (willcl-ark)
Pull request description:
In a similar vein to #28784, if a second `bitcoind` is started using the same datadir it will fail to start up, but during shutdown remove the PID file from the first `bitcoind` instance.
ACKs for top commit:
achow101:
ACK 8f6ab318635d823a7e6ad9020ac17b8d2dd0e91b
andrewtoth:
ACK 8f6ab318635d823a7e6ad9020ac17b8d2dd0e91b
romanz:
ACK 8f6ab31863
Tree-SHA512: c9af703cbfa179d33ef9580a51e86c1b0acbd28daa18c8d2e5e5ff796ab4d3e2009a962a47e6046a0e5ece936f8a06ee8af5fdf8ff4ae1e52cbcdbec4b942271
Previously, starting a second bitcoind using the same datadir would
correctly fail to init and shutdown. However during shutdown the PID
file belonging to the first instance would be erroneously removed by
the second process shutting down.
Fix this to only delete the PID file if we created it.
05aca093819be276ac7d648472c6ed5c7d235cc5 build: Patch Qt to handle minimum macOS version properly (Hennadii Stepanov)
Pull request description:
This PR is:
- required to [switch](https://github.com/bitcoin/bitcoin/pull/28622) to macOS 14 SDK (Xcode 15).
- an alternative to https://github.com/bitcoin/bitcoin/pull/28732 and https://github.com/bitcoin/bitcoin/pull/28775.
Qt relies on the `__MAC_OS_X_VERSION_MIN_REQUIRED` macro, which is set in the `AvailabilityInternal.h` SDK header to
the value provided by the Clang driver from the `-mmacos-version-min` / `-mmacosx-version-min` option.
Xcode 12 SDK expects the OS-specific `__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__` macro:
```c++
#ifndef __MAC_OS_X_VERSION_MIN_REQUIRED
#ifdef __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__
/* compiler for Mac OS X sets __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ */
#define __MAC_OS_X_VERSION_MIN_REQUIRED __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__
#endif
#endif /* __MAC_OS_X_VERSION_MIN_REQUIRED*/
```
In the other hand, Xcode 15 SDK expects a general `__ENVIRONMENT_OS_VERSION_MIN_REQUIRED__` macro:
```c++
#ifndef __MAC_OS_X_VERSION_MIN_REQUIRED
#if defined(__has_builtin) && __has_builtin(__is_target_os)
#if __is_target_os(macos)
#define __MAC_OS_X_VERSION_MIN_REQUIRED __ENVIRONMENT_OS_VERSION_MIN_REQUIRED__
#define __MAC_OS_X_VERSION_MAX_ALLOWED __MAC_14_0
#endif
#elif __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__
#define __MAC_OS_X_VERSION_MIN_REQUIRED __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__
#define __MAC_OS_X_VERSION_MAX_ALLOWED __MAC_14_0
#endif /* __has_builtin(__is_target_os) && __is_target_os(macos) */
#endif /* __MAC_OS_X_VERSION_MIN_REQUIRED */
```
The latter macro is not provided by LLVM Clang until c8e2dd8c6f, which is available in Clang 17.
The suggested patch makes Qt "borrow" the `__MAC_OS_X_VERSION_MIN_REQUIRED` value from `MAC_OS_X_VERSION_MIN_REQUIRED`, which is set in the `AvailabilityMacros.h` SDK header.
ACKs for top commit:
maflcko:
lgtm ACK 05aca093819be276ac7d648472c6ed5c7d235cc5
Tree-SHA512: 8891aefde4b8a48885abf0648f4ec71a22f7fcfca1e17ebb8c70ce1ef44751ea5db6b8b652de6ee8a716ca5f96f720fef01600bc23986162d0146c946e2e8743
97d53e90badc01224139a4a4356b934f338568d4 ci: remove libz-dev from macOS build deps (fanquake)
Pull request description:
Remove no-longer used libz-dev (from DMG compression).
Followup to #28432.
Missed this in #28932.
ACKs for top commit:
TheCharlatan:
ACK 97d53e90badc01224139a4a4356b934f338568d4
Tree-SHA512: ea4e46d83a11cfb0d77e7c5b370359864a026ca58e02de1a3d30458e4463dabd338db9546da63b782a7fa0ce199446c418fbfdfc400f3803b0ea9dd626aed42f
91504cbe0de2b74ef1aa2709761aaf0597ec66a2 rpc: `SyncWithValidationInterfaceQueue` on fee estimation RPC's (ismaelsadeeq)
714523918ba2b853fc69bee6b04a33ba0c828bf5 tx fees, policy: CBlockPolicyEstimator update from `CValidationInterface` notifications (ismaelsadeeq)
dff5ad3b9944cbb56126ba37a8da180d1327ba39 CValidationInterface: modify the parameter of `TransactionAddedToMempool` (ismaelsadeeq)
91532bd38223d7d04166e05de11d0d0b55e60f13 tx fees, policy: update `CBlockPolicyEstimator::processBlock` parameter (ismaelsadeeq)
bfcd401368fc0dc43827a8969a37b7e038d5ca79 CValidationInterface, mempool: add new callback to `CValidationInterface` (ismaelsadeeq)
0889e07987294d4ef2814abfca16d8e2a0c5f541 tx fees, policy: cast with static_cast instead of C-Style cast (ismaelsadeeq)
a0e3eb7549d2ba4dd3af12b9ce65e29158f59078 tx fees, policy: bugfix: move `removeTx` into reason != `BLOCK` condition (ismaelsadeeq)
Pull request description:
This is an attempt to #11775
This Pr will enable fee estimator to listen to ValidationInterface notifications to process new transactions added and removed from the mempool.
This PR includes the following changes:
- Added a new callback to the Validation Interface `MempoolTransactionsRemovedForConnectedBlock`, which notifies listeners about the transactions that have been removed due to a new block being connected, along with the height at which the transactions were removed.
- Modified the `TransactionAddedToMempool` callback parameter to include additional information about the transaction needed for fee estimation.
- Updated `CBlockPolicyEstimator` to process transactions using` CTransactionRef` instead of `CTxMempoolEntry.`
- Implemented the `CValidationInterface` interface in `CBlockPolicyEstimater` and overridden the `TransactionAddedToMempool`, `TransactionRemovedFromMempool`, and `MempoolTransactionsRemovedForConnectedBlock` methods to receive updates from their notifications.
Prior to this PR, the fee estimator updates from the mempool, i.e whenever a new block is connected all transactions in the block that are in our mempool are going to be removed using the `removeForBlock` function in `txmempool.cpp`.
This removal triggered updates to the fee estimator. As a result, the fee estimator would block mempool's `cs` until it finished updating every time a new block was connected.
Instead of being blocked only on mempool tx removal, we were blocking on both tx removal and fee estimator updating.
If we want to further improve fee estimation, or add heavy-calulation steps to it, it is currently not viable as we would be slowing down block relay in the process
This PR is smaller in terms of the changes made compared to #11775, as it focuses solely on enabling fee estimator updates from the validationInterface/cscheduler thread notifications.
I have not split the validation interface because, as I understand it, the rationale behind the split in #11775 was to have `MempoolInterface` signals come from the mempool and `CValidationInterface` events come from validation. I believe this separation can be achieved in a separate refactoring PR when the need arises.
Also left out some commits from #11775
- Some refactoring which are no longer needed.
- Handle reorgs much better in fee estimator.
- Track witness hash malleation in fee estimator
I believe they are a separate change that can come in a follow-up after this.
ACKs for top commit:
achow101:
ACK 91504cbe0de2b74ef1aa2709761aaf0597ec66a2
TheCharlatan:
Re-ACK 91504cbe0de2b74ef1aa2709761aaf0597ec66a2
willcl-ark:
ACK 91504cbe0de2b74ef1aa2709761aaf0597ec66a2
Tree-SHA512: 846dfb9da57a8a42458827b8975722d153907fe6302ad65748d74f311e1925557ad951c3d95fe71fb90ddcc8a3710c45abb343ab86b88780871cb9c38c72c7b1
7cb9367157eb42ee06bc6fa024522cc14a80138d rpc: keep .cookie if it was not generated (Roman Zeyde)
Pull request description:
Otherwise, starting bitcoind twice may cause the `.cookie` file generated by the first instance to be deleted by the second instance shutdown (after failing to obtain a lock).
ACKs for top commit:
willcl-ark:
re-ACK 7cb9367157eb42ee06bc6fa024522cc14a80138d
achow101:
ACK 7cb9367157eb42ee06bc6fa024522cc14a80138d
kristapsk:
re-ACK 7cb9367157eb42ee06bc6fa024522cc14a80138d
stickies-v:
ACK 7cb9367157eb42ee06bc6fa024522cc14a80138d
Tree-SHA512: 0960dbc457975b0e0535f3d814824a879d7f85c9f1191537415b3fc253429a316a8e4badde56c8bc139778f132392983cec5fbe03891fb15ff61d3bc3f6e681b
f23ba24aa079d68697d475789cd21bd7b5075550 test_submitpackage: only make a chain of 3 txns (Greg Sanders)
e67a345162912ef7c1bfa3c89c7e7c629505f0a3 doc: submitpackage vsize results are sigops-adjusted (Greg Sanders)
b67db52c399089e5d4c4202ebb905794dfd050d0 RPC submitpackage: change return format to allow partial errors (Greg Sanders)
Pull request description:
This was prompted by errors being returned that didn't "make any sense" to me, because it would for example return a "fee too low" error, when the "real" error was the child had something invalid, which disallowed CPFP evaluation. Rather than make judgment calls on what error is important(which is currently just return the "first"!), we simply return all errors and let the callers determine what's best.
Added a top level `package_msg` for quick eye-balling of general success of the package.
This PR also fixes a couple bugs:
1) Currently we don't actually broadcast a transaction, even if it was entered into our mempool, if a subsequent transaction causes `PKG_TX` failure.
2) "other-wtxid" is uncovered by tests, but IIUC was previously required to return "fees" and "vsize" results, but did not. I just make those results optional.
ACKs for top commit:
Sjors:
Light re-utACK f23ba24aa079d68697d475789cd21bd7b5075550
achow101:
ACK f23ba24aa079d68697d475789cd21bd7b5075550
glozow:
utACK f23ba24aa079d68697d475789cd21bd7b5075550, thanks for taking the suggestions
Tree-SHA512: ebfd716a4fed9e8c2dea3d2181ba6a6171b06718d29ac2324c67b7a30b374d199f7e1739f91ab5d036be172d0479de9bc89c32263ee62143c0338b9b622d0cca
fa98a097a30bc39f2424c0efd28a7979155faae6 Rename version.h to node/protocol_version.h (MarcoFalke)
fa4fbd58169a244c14017c62218e443b18a868ef Remove unused version.h include (MarcoFalke)
fa0ae22ff2c608c94b26c85040c4a1c7e9f7cf90 Remove unused SER_NETWORK, SER_DISK (MarcoFalke)
fae00fe9c25af80024adda33d9077962964269ea Remove unused CDataStream (MarcoFalke)
fa7eb4f5c3d2438f9689cd46b22dcfd50f6bd751 fuzz: Drop unused version from fuzz input format (MarcoFalke)
Pull request description:
Seems odd to have code that is completely dead.
Fix this by removing all of it.
ACKs for top commit:
sipa:
utACK fa98a097a30bc39f2424c0efd28a7979155faae6
ajtowns:
ACK fa98a097a30bc39f2424c0efd28a7979155faae6
ryanofsky:
Seems odd to not code review ACK fa98a097a30bc39f2424c0efd28a7979155faae6 (looks good)
Tree-SHA512: 9f1b9d9f92bda0512610bda6653e892756f637860362a9abfa439faab62de233cbad94b7df78ebacc160d9667aadfed4d9df08c0edefa618c040a049050fb913
6d05c4fd138b80168d14a8cf1dbcca43782851af msvc: Specify `boost-date-time` package explicitly (Hennadii Stepanov)
1f97e51d737c1a5a3e4503db3373a792f87f3bc1 msvc: Update vcpkg manifest baseline up to "2023.08.09 Release" (Hennadii Stepanov)
2d2ef2f14f352438afab2e3251d07fd100be7076 msvc: No need to specify the default feature for `libevent` package (Hennadii Stepanov)
Pull request description:
Last time we updated dependency packages used when compiling with MSVC in https://github.com/bitcoin/bitcoin/pull/26891. Then we [switched](https://github.com/bitcoin/bitcoin/pull/27335) to specifying the `builtin-baseline` in the `vcpkg.json` manifest file, which made checking out the entire vcpkg repository to a specific commit or tag unneeded.
This PR updates the manifest baseline from [2023.01.09](https://github.com/microsoft/vcpkg/releases/tag/2023.01.09) to [2023.08.09](https://github.com/microsoft/vcpkg/releases/tag/2023.08.09):
- berkeleydb: 4.8.30#8 --> 4.8.30#9
- boost: 1.81.0 --> 1.82.0#2
- sqlite3: 3.40.0#1 --> 3.42.0#1
- zeromq: 4.3.4#6 --> 2023-06-20#1
The most recent https://github.com/microsoft/vcpkg/releases/tag/2023.11.20 tag is still unavailable in the vcpkg installation in the current GHA Windows image (the head commit is 2b14b606ce only).
The other https://github.com/microsoft/vcpkg/releases/tag/2023.10.19 tag [introduces](0c42a58eac) a warning C4297 in the Boost.Process 1.83 that is [fixed](https://github.com/boostorg/process/pull/340) in the upcoming version 1.84.
ACKs for top commit:
fanquake:
ACK 6d05c4fd138b80168d14a8cf1dbcca43782851af
Tree-SHA512: c71c46c13ad5a6d39ae72982b7765f690e7391092e4983887a53dceeee507c8fc256e887351bbe35c051bf88d2babeb283e17a2588ee86ec9020c4ba426bfcd7
e67634ef19db310511a22f461bb1af7edb3d862b fuzz: BIP324: damage ciphertext/aad in full byte range (Sebastian Falbesoner)
Pull request description:
This PR is a tiny improvement for the `bip324_cipher_roundtrip` fuzz target: currently the damaging of input data for decryption (either ciphertext or aad) only ever happens in the lower nibble within the byte at the damage position, as the bit position for the `damage_val` byte was calculated with `damage_bit & 3` (corresponding to `% 4`) rather than `damage_bit & 7` (corresponding to the expected `% 8`).
Noticed while reviewing #28263 which uses similar constructs.
ACKs for top commit:
stratospher:
ACK e67634ef.
dergoegge:
utACK e67634ef19db310511a22f461bb1af7edb3d862b
Tree-SHA512: 1bab4df28708e079874feee939beef45eff235215375c339decc696f4c9aef04e4b417322b045491c8aec6e88ec8ec2db564e27ef1b0be352b6ff4ed38bad49a
fad82fea2bef88c9f11e25ca43c7886a2b9b5da2 ci: Reduce use of bash -c (MarcoFalke)
fafcee48748d4a4ff229ae94d2bc3a73c5c1db22 ci: Rename test script to 03_test_script.sh (MarcoFalke)
Pull request description:
It is confusing to treat commands as a single string. This change is
also required to support paths and strings with spaces in them in the
future.
ACKs for top commit:
RandyMcMillan:
utACK fad82fea2bef88c9f11e25ca43c7886a2b9b5da2
vasild:
ACK fad82fea2bef88c9f11e25ca43c7886a2b9b5da2
Tree-SHA512: fb79469d809400739e53da203842fda838f2ec9ab8dcd5e622ccd3db651d629161323bfcc04301562f13f5c407e8865036478a4ac7f6b5265dc4dda1a320c23d
66c4b58e518aff08030b3c879c44af7716110619 guix: switch from guix environment to guix shell (fanquake)
Pull request description:
See https://guix.gnu.org/manual/devel/en/html_node/Invoking-guix-environment.html.
> Deprecation warning: The guix environment command is deprecated
in favor of guix shell, which performs similar functions but is more convenient to use. See Invoking guix shell.
> Being deprecated, guix environment is slated for eventual removal,
but the Guix project is committed to keeping it until May 1st, 2023. Please get in touch with us at guix-devel@gnu.org if you would like to discuss it.
See also https://guix.gnu.org/blog/2021/from-guix-environment-to-guix-shell/ for a blog post and additional details.
Guix `shell` was added to Guix ~1 year ago, in this commit, https://git.savannah.gnu.org/cgit/guix.git/commit/?id=80edb7df6586464aa40e84e103f0045452de95db, which isn't part of the 1.3.0 release binaries out of the box, but invoking a `guix pull`, and updating will make it available. i.e:
```bash
bash-5.1# guix --version
guix (GNU Guix) 1.3.0
Copyright (C) 2021 the Guix authors
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
bash-5.1# guix shell
guix: shell: command not found
Try 'guix --help' for more information.
bash-5.1# guix pull
Updating channel 'guix' from Git repository at 'https://git.savannah.gnu.org/git/guix.git'...
Authenticating channel 'guix', commits 9edb3f6 to 7a980bb (6,278 new commits)...
Building from this channel:
guix https://git.savannah.gnu.org/git/guix.git7a980bb
< snip >
building /gnu/store/2wwwsczxcw61m05p4mv0kf0advx4fqsb-inferior-script.scm.drv...
building package cache...
building profile with 1 package...
New in this revision:
6,866 new packages: a2jmidid, abjad,
bash-5.1# guix help shell
Usage: guix shell [OPTION] PACKAGES... [-- COMMAND...]
Build an environment that includes PACKAGES and execute COMMAND or an
interactive shell in that environment.
```
ACKs for top commit:
TheCharlatan:
ACK 66c4b58e518aff08030b3c879c44af7716110619
Tree-SHA512: caa3fd2ca8d0f261c50ecdda3728a75389d24d89b51293dedc704ee77ab1342b2bb08ca8c871dcb4646229f056ec86cb15500934ded1b0c501a3ffc25aaa8ae6
Behavior prior to this commit allows some transactions to
enter into the local mempool but not be reported to the user
when encountering a PackageValidationResult::PCKG_TX result.
This is further compounded with the fact that any transactions
submitted to the mempool during this call would also not be
relayed to peers, resulting in unexpected behavior.
Fix this by, if encountering a package error, reporting all
wtxids, along with a new error field, and broadcasting every
transaction that was found in the mempool after submission.
Note that this also changes fees and vsize to optional,
which should also remove an issue with other-wtxid cases.
faecde9102f2e4199df3ec760f9378b114a87531 fuzz: Fix nullptr deref in scriptpubkeyman (MarcoFalke)
Pull request description:
This should fix the UB that was found by review (https://github.com/bitcoin/bitcoin/pull/28578#discussion_r1404246109) and by fuzzing (https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=64487)
ACKs for top commit:
dergoegge:
utACK faecde9102f2e4199df3ec760f9378b114a87531
brunoerg:
crACK faecde9102f2e4199df3ec760f9378b114a87531
Tree-SHA512: ff726ed632d8d369c96d316bafebe87ff385e47b74b1d1da79409ddf296559eb991431883858057527e5df2414c01812ecbc99c21c69020228b0747f32b03121
Also, add missing includes to scriptpubkeyman.
Also, export dependecies of the BasicTestingSetup from setup_common.h,
to avoid having to include them when setup_common.h is already included.
It is confusing to treat commands as a single string. This change is
also required to support paths and strings with spaces in them in the
future.
This requires replacing TEST_RUNNER_ENV with a global export, because it
no longer works. See:
```bash
$ export ENV="A=1" && $ENV ls
bash: A=1: command not found...
```
Or in the CI task:
+ DIR_UNIT_TEST_DATA=/ci_container_base/ci/scratch/qa-assets/unit_test_data/
+ LD_LIBRARY_PATH=/ci_container_base/depends/i686-pc-linux-gnu/lib
+ BITCOIND=bitcoin-node make -j10 check VERBOSE=1
/ci_container_base/ci/test/03_test_script.sh: line 166: BITCOIND=bitcoin-node: command not found
https://github.com/bitcoin/bitcoin/pull/28954/checks?check_run_id=19096858944https://cirrus-ci.com/task/6718317604372480
fa02c08c93e5867b7ea07d79ca1c0917dcde88e0 refactor: Use Txid in CMerkleBlock (MarcoFalke)
Pull request description:
This should also fix a gcc-13 compiler warning, see https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1407856376
```
rpc/txoutproof.cpp: In lambda function:
rpc/txoutproof.cpp:72:33: error: possibly dangling reference to a temporary [-Werror=dangling-reference]
72 | const Coin& coin = AccessByTxid(active_chainstate.CoinsTip(), Txid::FromUint256(tx));
| ^~~~
rpc/txoutproof.cpp:72:52: note: the temporary was destroyed at the end of the full expression ‘AccessByTxid((*(const CCoinsViewCache*)(&(& active_chainstate)->Chainstate::CoinsTip())), transaction_identifier<false>::FromUint256((* & tx)))’
72 | const Coin& coin = AccessByTxid(active_chainstate.CoinsTip(), Txid::FromUint256(tx));
| ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
ACKs for top commit:
TheCharlatan:
Re-ACK fa02c08c93e5867b7ea07d79ca1c0917dcde88e0
dergoegge:
reACK fa02c08c93e5867b7ea07d79ca1c0917dcde88e0
Tree-SHA512: 2e6837b9d0c90bd6e9d766330e7086d68c6ec80bb27fe2cfc4702b251b00d91a79f8bfbc76d998cbcd90bee5317402cf617f61099eee96d94e7ac8f37ba7a642
fa1a38470697796a1a67397a815c8f8256f59224 Move compat.h include from system.h to system.cpp (MarcoFalke)
88887531b704f3943fdb33abbdd5378ecfeee14f Move compat/assumptions.h include to one place that actually needs it (MarcoFalke)
77774110f4dd591a71441851813d59c03c9e3c78 Remove __cplusplus from compat/assumptions.h (MarcoFalke)
faa3d4f1d8ecff444be53215d72e32d71d9ce138 Remove duplicate NDEBUG check from compat/assumptions.h (MarcoFalke)
Pull request description:
Generally, compile-time checks should be close to the code that use them. Especially, since `compat/assumptions.h` is only included in one place, where iwyu suggests to remove it.
Fix all issues:
* The `NDEBUG` check is used in `util/check`, so it is redundant in `compat/assumptions.h`.
* The `__cplusplus` check is redundant with `doc/dependencies.md` (see commit message).
* Add missing `// IWYU pragma: keep` to avoid removing the include by accident.
ACKs for top commit:
achow101:
ACK fa1a38470697796a1a67397a815c8f8256f59224
TheCharlatan:
re-ACK fa1a38470697796a1a67397a815c8f8256f59224
theuni:
ACK fa1a38470697796a1a67397a815c8f8256f59224
Tree-SHA512: f8b6db84be5d8844a2267345c0b1405fcbc39b8b5eeaa24db5b8412a74145fe44cf188b6b0c39cc2b062690ed37ca5b4662473484afe28dbec6469e79961389b
9ac114e5cd9d8ade3a1d9f3d76a08ff59a3f1658 Throw error if invalid parameters passed to getnetworkhashps RPC endpoint (Jameson Lopp)
Pull request description:
When writing some scripts that iterated over many blocks to generate hashrate estimates I realized that my script was going out of range of the current chain tip height but was not encountering any errors.
I believe that passing an invalid block height to this function but receiving the hashrate estimate for the chain tip instead should be considered unexpected behavior.
ACKs for top commit:
Sjors:
re-utACK 9ac114e5cd9d8ade3a1d9f3d76a08ff59a3f1658
kevkevinpal:
reACK [9ac114e](9ac114e5cd)
achow101:
ACK 9ac114e5cd9d8ade3a1d9f3d76a08ff59a3f1658
Tree-SHA512: eefb465c2dd654fc48267f444e1809597ec5363cdd131ea9ec812458fed1e4bffbbbb0617d74687c9f7bb16274b598d8292f5eeb7953421e5d2a8dc2cc081f2b
705e3f1de00bf30d728addd52a790a139d948e32 refactor: Make CTxMemPoolEntry only explicitly copyable (TheCharlatan)
Pull request description:
This has the goal of prohibiting users from accidentally creating runtime failures, e.g. by interacting with iterator_to with a copied entry. This was brought up here: https://github.com/bitcoin/bitcoin/pull/28886#issuecomment-1814794954.
CTxMemPoolEntry is already implicitly not move-constructable. So be explicit about this and use a std::list to collect the values in the policy_estimator fuzz test instead of a std::vector.
ACKs for top commit:
maflcko:
ACK 705e3f1de00bf30d728addd52a790a139d948e32 🌯
achow101:
ACK 705e3f1de00bf30d728addd52a790a139d948e32
ajtowns:
ACK 705e3f1de00bf30d728addd52a790a139d948e32
ismaelsadeeq:
ACK 705e3f1de00bf30d728addd52a790a139d948e32
Tree-SHA512: 62056905c679c919d00f9ae065ed66ac986e7e7062015aea542843d8deecda57104d7a68d002f7b20afa3164f8e9215d2d2d002c167224129540e3b1bd0712cc
35fb9930adb3501b29d3ad20d2e74c0114f2bcbe test: enable v2 transport for p2p_timeouts.py (Martin Zumsande)
2c1669c37a9759e15ff5f4e340aeaa8778a81b9a test: enable v2 transport for rpc_net.py (Sebastian Falbesoner)
cc961c26956859850202f56191981b0306a65fcf test: enable v2 transport for p2p_node_network_limited.py (Sebastian Falbesoner)
3598a1b5c932634dc7ccb991cc83df5e1a1dcaa9 test: enable --v2transport in combination with --usecli (Martin Zumsande)
68a90017519874793e34e3b439a63e5aa3a6f6a7 test: persist -v2transport over restarts and respect -v2transport=0 (Martin Zumsande)
Pull request description:
This makes the functional test suite compatible with BIP324, so that
`python3 test_runner.py --v2transport`
should succeed (currently, 12 tests fail for me on master).
Includes two commits by TheStack I found in an old discussion https://github.com/bitcoin/bitcoin/pull/28331#discussion_r1326714164
Note that even though all tests should pass, the python `p2p.py` module will do v2 connections only after the merge of #24748, so that for now only connections between two full nodes will actually run v2.
Some of the fixed tests were added with `--v2transport` to the test runner. Though after #24748 we might also want to consider running the entire suite with `--v2transport` in some CI.
ACKs for top commit:
sipa:
utACK 35fb9930adb3501b29d3ad20d2e74c0114f2bcbe. Thanks for taking care of this.
achow101:
ACK 35fb9930adb3501b29d3ad20d2e74c0114f2bcbe
theStack:
ACK 35fb9930adb3501b29d3ad20d2e74c0114f2bcbe
stratospher:
ACK 35fb993.
Tree-SHA512: 80dc0bf211fa525ff1d092043aea9f222f14c02e5832a548fb8b83b9ede1fcee03c5e8ade0d05c331bdaa492af9c1cf3d0f0b15b846673c6eacea82dd4cefbc3
fae76a1f2ac0b352cdd708e6d57f8605bf40112e scripted-diff: Use DataStream in most places (MarcoFalke)
fac39b56b723f79ba0aa9570708a1d4a600f4e12 refactor: SpanReader without nVersion (MarcoFalke)
Pull request description:
The serialize version is unused, so remove it. This also allows to remove `GCS_SER_VERSION` and allows a scripted-diff to remove most of `CDataStream`.
ACKs for top commit:
ajtowns:
ACK fae76a1f2ac0b352cdd708e6d57f8605bf40112e
ryanofsky:
Code review ACK fae76a1f2ac0b352cdd708e6d57f8605bf40112e
Tree-SHA512: 3b487dba8ea380f1eacff9fdfb9197f025dbc30906813d3f4c3e6f1e9e4d9f2a169c6f163f51d135e18af538be78e2d2b13d694073ad25c5762980ae971a4c83
