Add an option to keep building the template regardless of feerate. We
can't just use target_feerate=0 because it's possible for transactions
to have negative modified feerates.
No behavior change for users that pass in a target_feerate.
This is primarily intended for linearizing a package of transactions
prior to submitting them to mempool. Note that, if this ctor is used,
bump fees will not be calculated because we haven't instructed MiniMiner
which outpoints for which we want bump fees to be calculated.
No behavior change. All we are doing is copying out these values before
passing them into the ctor instead of within the ctor.
This makes it possible to use the MiniMiner algorithms to analyze
transactions that haven't been submitted to the mempool yet.
It also iwyu's the mini_miner includes.
bb91131d54 doc: remove out-of-date external link in src/util/strencodings.h (Jon Atack)
7d494a48dd refactor: use string_view to pass string literals to Parse{Hash,Hex} (Jon Atack)
Pull request description:
as `string_view` is optimized to be trivially copiable, whereas the current code creates a `std::string` copy at each call.
These utility methods are called by quite a few RPCs and tests, as well as by each other.
```
$ git grep "ParseHashV\|ParseHashO\|ParseHexV\|ParseHexO" | wc -l
61
```
Also remove an out-of-date external link.
ACKs for top commit:
jonatack:
Rebased per `git range-diff c9273f6 b94581a bb91131` for an include header from the merge of https://github.com/bitcoin/bitcoin/pull/28230. Should be trivial to re-ACK.
maflcko:
lgtm ACK bb91131d54
ns-xvrn:
ACK bb91131d54
achow101:
ACK bb91131d54
brunoerg:
crACK bb91131d54
Tree-SHA512: 9734fe022c9e43fd93c23a917770d332dbbd3132c80a234059714c32faa6469391e59349954749fc86c4ef0b18d5fd99bf8f4b7b82d9f799943799c1253272ae
376dc2cfb3 test: add coverage to rpc_blockchain.py (kevkevin)
Pull request description:
Included a test that checks the functionality of setting
the first param of getnetworkhashps to negative value returns
the average network hashes per second from the last difficulty change.
ACKs for top commit:
jlopp:
tACK 376dc2cfb3
achow101:
ACK 376dc2cfb3
ismaelsadeeq:
Tested ACK 376dc2cfb3
pablomartin4btc:
tACK 376dc2cfb3
Tree-SHA512: 02d52f622e9cb7a1240c5d124510dd75d03f696f119b2625b0befd60b004ec50ff1a2d5515e0e227601adeecd837e0778ed131ee2a8c5f75f1b824be711213a7
37d150d8c5 refactor: Add more negative `!m_banned_mutex` thread safety annotations (Hennadii Stepanov)
0fb2908708 refactor: replace RecursiveMutex m_banned_mutex with Mutex (w0xlt)
784c316f9c scripted-diff: rename m_cs_banned -> m_banned_mutex (w0xlt)
46709c5f27 refactor: Get rid of `BanMan::SetBannedSetDirty()` (Hennadii Stepanov)
d88c0d8440 refactor: Get rid of `BanMan::BannedSetIsDirty()` (Hennadii Stepanov)
Pull request description:
This PR is an alternative to bitcoin/bitcoin#24092. Last two commit have been cherry-picked from the latter.
ACKs for top commit:
maflcko:
ACK 37d150d8c5🎾
achow101:
ACK 37d150d8c5
theStack:
Code-review ACK 37d150d8c5
vasild:
ACK 37d150d8c5
Tree-SHA512: 5e9d40101a09af6e0645a6ede67432ea68631a1b960f9e6af0ad07415ca7718a30fcc1aad5182d1d5265dc54c26aba2008fc9973840255c09adbab8fedf10075
Clarify that supported versions of GCC are not affected, and that Clang
prior to version 15 still requires the explicit -latomic linking, when
compiling for 32-bit.
a5e39d325d Fee estimation: extend bucket ranges consistently (Anthony Towns)
Pull request description:
When calculating a median fee for a confirmation target at a particular threshold, we analyse buckets in ranges rather than individually in case some buckets have very little data. This patch ensures the breaks between ranges are independent of the the confirmation target.
Fixes#20725
ACKs for top commit:
ismaelsadeeq:
Code review ACK a5e39d325d
glozow:
btw what I meant by [this](https://github.com/bitcoin/bitcoin/pull/21161#pullrequestreview-1350258467) was ACK a5e39d325d
jonatack:
Initial ACK a5e39d325d
Tree-SHA512: 0edf4e56717c4ab8d4ab0bc0f1d7ab36a13b99de12f689e55c9142c6b81691367ffd8df2e8260c5e14335310b1a51770c6c22995db31109976239befcb558ef8
9b3da70bd0 [test] DisconnectedBlockTransactions::DynamicMemoryUsage (glozow)
b2d0447964 bugfix: correct DisconnectedBlockTransactions memory usage (stickies-v)
f4254e2098 assume duplicate transactions are not added to `iters_by_txid` (ismaelsadeeq)
29eb219c12 move only: move implementation code to disconnected_transactions.cpp (ismaelsadeeq)
81dfeddea7 refactor: update `MAX_DISCONNECTED_TX_POOL` from kb to bytes (ismaelsadeeq)
Pull request description:
This PR is a follow-up to fix review comments and a bugfix from #28385
The PR
- Updated `DisconnectedBlockTransactions`'s `MAX_DISCONNECTED_TX_POOL` from kb to bytes.
- Moved `DisconnectedBlockTransactions` implementation code to `kernel/disconnected_transactions.cpp`.
- `AddTransactionsFromBlock` now assume duplicate transactions are not passed by asserting after inserting each transaction to `iters_by_txid`.
- Included a Bug fix: In the current master we are underestimating the memory usage of `DisconnectedBlockTransactions`.
* When adding and subtracting `cachedInnerUsage` we call `RecursiveDynamicUsage` with `CTransaction` which invokes this [`RecursiveDynamicUsage(const CTransaction& tx)`](6e721c923c/src/core_memusage.h (L32)) version of `RecursiveDynamicUsage`, the output of that call only account for the memory usage of the inputs and outputs of the `CTransaction`, this omits the memory usage of the `CTransaction` object and the control block.
* This PR fixes this bug by calling `RecursiveDynamicUsage` with `CTransactionRef` when adding and subtracting `cachedInnerUsage` which invokes [`RecursiveDynamicUsage(const std::shared_ptr<X>& p)`](6e721c923c/src/core_memusage.h (L67)) version of `RecursiveDynamicUsage` the output of the calculation accounts for the` CTransaction` object, the control blocks, inputs and outputs memory usage.
* see [comment ](https://github.com/bitcoin/bitcoin/pull/28385#discussion_r1322948452)
- Added test for DisconnectedBlockTransactions memory limit.
ACKs for top commit:
stickies-v:
ACK 9b3da70bd0 - nice work!
BrandonOdiwuor:
re ACK 9b3da70bd0
glozow:
ACK 9b3da70bd0
Tree-SHA512: 69b9595d09f4d0209038f97081d790cea92ccf63efb94e9e372749979fcbe527f7f17a8e454720cedd12021be0c8e11cf99874625d3dafd9ec602b12dbeb4098
8047bb6fea build: Update `qt` package up to 5.15.11 (Hennadii Stepanov)
Pull request description:
In the light of https://github.com/bitcoin/bitcoin/pull/28622, we probably have to patch Qt. It seems reasonable to update it up to the latest available version before doing that.
ACKs for top commit:
TheCharlatan:
ACK 8047bb6fea
Tree-SHA512: b4d7df2ff059b8f58c3202d913237c0d39a962748658f1ce853884dca095fbda5f56d4d68f73a1bc8da2f295e96a20927306e148b41a9f4afc42c8edb11c3729
Support the creation of a transaction with multiple specified inputs or
outputs. Also accept a target feerate and return the fee paid.
Also, signal BIP125 by default - a subsequent commit needs to RBF
something.
Co-authored-by: Andrew Chow <achow101@gmail.com>
This allows IsSorted() and IsConsistent() to be used by themselves.
IsSorted() with a precomputed set is used so that we don't create this
set multiple times.
9cfc1c9440 test: check that we don't send a getaddr msg to an inbound peer (Martin Zumsande)
88c33c6748 test: make python p2p not send getaddr messages when it's being connected to (Martin Zumsande)
Pull request description:
`bitcoind` nodes send `getaddr` messages only to outbound nodes (and ignore `getaddr` received by outgoing connections).
The python p2p node should mirror this behavior by not sending a `getaddr` message when it is not the initiator of the connection.
This is currently causing several unnecessary messages being sent and then ignored (`Ignoring "getaddr" from outbound-full-relay connection.`) in tests like `p2p_add_connections.py`.
ACKs for top commit:
pinheadmz:
concept ACK 9cfc1c9440
pablomartin4btc:
re ACK 9cfc1c9440
BrandonOdiwuor:
re ACK 9cfc1c9440
Tree-SHA512: 812bec5d8a4828b4384d4cdd4362d6eec09acb2363e888f2b3e3bf8b925e0e17f15e13dc297d6b616c68b93ace9ede7245b07b405d3f5f8eada98350f74230dc
02a4f1a385 addrman: log AS only when using asmap (brunoerg)
Pull request description:
This PR changes the log to just print the ASN when using asmap, same logic presented in other logs:
afa081a39b/src/net_processing.cpp (L3552-L3556)afa081a39b/src/net_processing.cpp (L3598-L3604)
ACKs for top commit:
naumenkogs:
ACK 02a4f1a385
mzumsande:
Code Review ACK 02a4f1a385
Tree-SHA512: adad5904ab163660d47554b32dc2dc3dfdff8dd64b94e5320ad11706381264d1e338654fa8239430eed4ccbebc8f6670698b4278895794055c37fc4bcefe71bc
b74e449ffa build: remove potential for duplciate natpmp linking (fanquake)
4e95096952 build: remove duplicate -lminiupnpc linking (fanquake)
Pull request description:
Having the link check in the header check loop means we get `-lminiupnpc -lminiupnpc -lminiupnpc` on the link line.
This is unnecessary, and results in warnings, i.e:
```bash
ld: warning: ignoring duplicate libraries: '-levent', '-lminiupnpc'
ld: warning: ignoring duplicate libraries: '-levent', '-lminiupnpc'
ld: warning: ignoring duplicate libraries: '-levent', '-lminiupnpc'
```
These warnings have been occurring since the new macOS linker released with Xcode 15, and also came up in https://github.com/hebasto/bitcoin/pull/34.
There are other duplicate lib issues, i.e with `-levent` + `-levent_pthreads -levent`, but those are less straight forward to solve, and won't be included here.
ACKs for top commit:
jonatack:
ACK b74e449ffa
hebasto:
ACK b74e449ffa, it fixes one issue mentioned in https://github.com/hebasto/bitcoin/pull/34#issuecomment-1782914787.
TheCharlatan:
ACK b74e449ffa
theuni:
ACK b74e449ffa
Tree-SHA512: 987a56ef17cbaf273cb672c41016f3f615b16889317325a9e88135d0c41f01af3840ad44a6f811a7df97f5873c9cd957e60aaa1b99bd408b17b4b1ffe2c68f36
e26e665f9f gui: fix crash on selecting "Mask values" in transaction view (Sebastian Falbesoner)
Pull request description:
This PR fixes a crash bug that can be caused with the following steps:
- change to the "Transactions" view
- right-click on an arbitrary transaction -> "Show transaction details"
- close the transaction detail window again
- select menu item "Settings" -> "Mask values"
The problem is that the list of opened dialogs, tracked in the member variable `m_opened_dialogs` (introduced in https://github.com/bitcoin-core/gui/pull/708, commit 4492de1be1), is only ever appended with newly opened transaction detail dialog pointers, but never removed. This leads to dangling pointers in the list, and if the "Mask values" menu item is selected, a crash is caused in the course of trying to close the opened transaction detail dialogs (see `closeOpenedDialogs()` method). Fix this by removing a pointer of the list if the corresponding widget is destroyed.
ACKs for top commit:
achow101:
ACK e26e665f9f
pablomartin4btc:
tACK e26e665f9f
furszy:
utACK e26e665f9
hebasto:
ACK e26e665f9f, tested on Ubuntu 22.04.
Tree-SHA512: 37885c22abae0ab065b4878bae46fd362f41b09609d081fd59e26bb05474f427b98771ee73f5480526afaef04e016c5ba62c956e0e85a57b6a0f44a905b68a83
for initial partial unit test coverage of these CConnman class methods:
- AddNode()
- ConnectNode()
- GetAddedNodeInfo()
- AlreadyConnectedToAddress()
- ThreadOpenAddedConnections()
and of the GetAddedNodeInfo() call in RPC addnode.
`send(2)` can be interrupted or for another reason it may not fully
complete sending all the bytes. We should be ready to retry the send
with the remaining bytes. This is what `Sock::SendComplete()` does,
thus use it in `Socks5()`.
Since `Sock::SendComplete()` takes a `CThreadInterrupt` argument,
change also the recv part of `Socks5()` to use `CThreadInterrupt`
instead of a boolean.
Easier reviewed with `git show -b` (ignore white-space changes).
79539fbfbf guix: update signapple (fanquake)
Pull request description:
Fixes#28449, and removes the need to boostrap Rust, by avoiding the `python-requests` dependency.
Comparing a `--no-substitutes` build of this PR, to master, signapple requires ~1350 _less_ packages to boostrap:
Master derivation - https://gist.github.com/fanquake/dbf69a62c9a78b7ae8c183a160e6d58d
PR derivation - https://gist.github.com/fanquake/0aa2d8eddaba861ba489ed3d936f727d
ACKs for top commit:
achow101:
ACK 79539fbfbf
Tree-SHA512: 341ddcae27e53c31d114465cb5173573dcc9e1c0874ee160715630f686da6f69255f6080ec0181ffeffc26efbdb545599d667784b1cd17dfa7e3da0998ec9bd6
f6f18eeaa8 guix: Zip needs to include all files with time as SOURCE_DATE_EPOCH (Andrew Chow)
Pull request description:
The zip for codesigned MacOS distribution needs to have all files included and have their timestamps set to the same value (`SOURCE_DATE_EPOCH`).
This uses the same pattern for zip as is done for the other zip files produced by guix.
ACKs for top commit:
hebasto:
ACK f6f18eeaa8.
TheCharlatan:
ACK f6f18eeaa8
Tree-SHA512: 569ff0d8bfe76b9b111a2454478523eeb514b44b691be8b57b61415db88356c683582550ea67ebd5fb392b4f486be170a925067b507979090535ca41cbc7351b
The zip for codesigned MacOS distribution needs to have all files have
the same timestamp. These files also need to be included in the zip as
zip is not automatically recursive. We use the same pattern for zip as
is done for the other zip files produced by guix.
Having the link check in the header check loop means we get `-lminiupnpc
-lminiupnpc -lminiupnpc` on the link line. This is unnecessary, and
results in warnings, i.e:
```bash
ld: warning: ignoring duplicate libraries: '-levent', '-lminiupnpc'
ld: warning: ignoring duplicate libraries: '-levent', '-lminiupnpc'
ld: warning: ignoring duplicate libraries: '-levent', '-lminiupnpc'
```
These warnings have been occurring since the new linker released with
Xcode 15, and also came up in https://github.com/hebasto/bitcoin/pull/34.
99990194ce Remove WithParams serialization helper (MarcoFalke)
ffffb4af83 scripted-diff: Use ser params operator (MarcoFalke)
fae9054793 test: Use SER_PARAMS_OPFUNC in serialize_tests.cpp (MarcoFalke)
Pull request description:
Every serialization parameter struct already has the `SER_PARAMS_OPFUNC`, except for one in the tests.
For consistency, and to remove verbose code, convert the test to `SER_PARAMS_OPFUNC`, and use it everywhere, then remove the `WithParams` helper.
ACKs for top commit:
ajtowns:
reACK 99990194ce
TheCharlatan:
Re-ACK 99990194ce
Tree-SHA512: be9cae4225a502486fe8d552aaf4b2cd2904a9f73cce9d931c6b7c757594ff1982fcc2c30d00d012cd12b0a9531fd609f8bcd7c94b811e965ac087eb8a3589d3
This commits fixes a crash bug that can be caused with the following steps:
- change to the "Transactions" view
- right-click on an arbitrary transaction -> "Show transaction details"
- close the transaction detail window again
- select "Settings" -> "Mask values"
The problem is that the list of opened dialogs, tracked in the member
variable `m_opened_dialogs`, is only ever appended with newly opened
transaction detail dialog pointers, but never removed. This leads to
dangling pointers in the list, and if the "Mask values" menu item is
selected, a crash is caused in the course of trying to close the opened
transaction detail dialogs (see `closeOpenedDialogs()` method). Fix this
by removing the pointer from the list if the corresponding widget is
destroyed.
fa56067a8f refactor: Fix bugprone-string-constructor warning (MarcoFalke)
Pull request description:
String literals in C++ have a trailing null character, so the current code is fine to rely on that implicitly. However,
* the sqlite documentation explicitly mentions the null character
* code readers may wonder if the code is intentional
* clang-tidy warns about the code via `bugprone-string-constructor`
Address the points by putting the null character into the code and enable the clang-tidy `bugprone-string-constructor` check.
ACKs for top commit:
stickies-v:
ACK fa56067a8f
Tree-SHA512: da519184d792a885a8151ffc44c8da5781f5aaae12ef768a187cc6d9e542ca8952aebc2ec6c1a05f673f29a86ef44902ee96e7b491af7b4705ad38e14624882e
that are otherwise private:
- CConnman::m_nodes
- CConnman::ConnectNodes()
- CConnman::AlreadyConnectedToAddress()
and update the #include headers per iwyu.
`CConnman::GetAddedNodeInfo` is used both to get a list of addresses to manually connect to
in `CConnman::ThreadOpenAddedConnections`, and to report about manually added connections in
`getaddednodeinfo`. In both cases, all addresses added to `m_added_nodes` are returned, however
the nodes we are already connected to are only relevant to the latter, in the former they are
actively discarded.
Parametrizes `CConnman::GetAddedNodeInfo` so we can ask for only addresses we are not connected to,
to avoid passing useless information around.
Currently it is possible to add the same node twice when formatting IPs in
different, yet equivalent, manner. This applies to both ipv4 and ipv6, e.g:
127.0.0.1 = 127.1 | [::1] = [0:0:0:0:0:0:0:1]
`addnode` will accept both and display both as connected (given they translate to
the same IP). This will not result in multiple connections to the same node, but
will report redundant info when querying `getaddednodeinfo` and populate `m_added_nodes`
with redundant data.
This can be avoided performing comparing the contents of `m_added_addr` and the address
to be added as `CServices` instead of as strings.
The current `addnode` rpc command has some edge cases in where it is possible to
connect to the same node twice by combining ip and address requests. This can happen under two situations:
The two commands are run one right after each other, in which case they will be processed
under the same loop in `CConnman::ThreadOpenAddedConnections` without refreshing `vInfo`, so both
will go trough. An example of this would be:
```
bitcoin-cli addnode "localhost:port" add
```
A node is added by IP using `addnode "add"` while the other is added by name using
`addnode "onetry"` with an address that resolves to multiple IPs. In this case, we currently
only check one of the resolved IPs (picked at random), instead of all the resolved ones, meaning
this will only probabilistically fail/succeed. An example of this would be:
```
bitcoin-cli addnode "127.0.0.1:port" add
[...]
bitcoin-cli addnode "localhost:port" onetry
```
Both cases can be fixed by iterating over all resolved addresses in `CConnman::ConnectNode` instead
of picking one at random
fa5423b5b5 refactor: Remove unused gcc-9 workaround in txrequest (MarcoFalke)
fa918d397d Always enable -Wsuggest-override (MarcoFalke)
faea58eee4 Bump g++ minimum supported version to 10 (MarcoFalke)
Pull request description:
All supported operating systems ship with g++ 10 (or later), so bumping the minimum should not cause any issues. The bump allows to drop some now-unused workarounds.
For reference:
* https://packages.debian.org/bullseye/g++ (`g++-10`)
* https://packages.ubuntu.com/focal/g++-10
* FreeBSD 12/13 ships with g++ 12
* CentOS-like 9 ships with g++ 11
* OpenSuse Tumbleweed ships with g++ 13 https://software.opensuse.org/package/gcc13-c++ (No idea about OpenSuse Leap)
This is for Bitcoin Core 27.0 in 2024 (next year), not the soon upcoming 26.0 next month.
ACKs for top commit:
fanquake:
ACK fa5423b5b5
Tree-SHA512: 6f0697ae4c0f578873591b7872bf158aba3af17f171c3556b593a70ec379bf94c7a9dd7697e8e79173edd4ac3c81a376e0cbbc0cfabde1a1cfe5f9b5eaea6831
faa769db5a Fix bugprone-lambda-function-name errors (MarcoFalke)
Pull request description:
Inside a lambda, `__func__` will evaluate to something like `"operator()"`. Fix this by either removing it, or by using the real name.
https://clang.llvm.org/extra/clang-tidy/checks/bugprone/lambda-function-name.html
ACKs for top commit:
TheCharlatan:
ACK faa769db5a
darosior:
utACK faa769db5a
Tree-SHA512: 0b562bd4ebd7f46ca3ebabeee67851ad30bd522fa57e5010e833b163664e51f5df645ff9ca35d22c3479fb27d9267d4e5d0d417d42729bf3ccf80d7944970e4e
