💬 ajtowns commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1190497125)
`MAX_PEER_TX_ANNOUNCEMENTS` doesn't really provide a rate limit -- it stops the queue from being more than 5k at any point in time, but the flow through the queue could still be 400tx/s, for instance. Beyond that, `m_recently_announced_invs` only tracks things we announced, not things we received, and we rate limit that (as per the static_assert on `INVENTORY_MAX_RECENT_RELAY`).
Delaying MEMPOOL results for 2 minutes seems annoying. But how about just recording the timestamp of the last MEMP
...
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1190497125)
`MAX_PEER_TX_ANNOUNCEMENTS` doesn't really provide a rate limit -- it stops the queue from being more than 5k at any point in time, but the flow through the queue could still be 400tx/s, for instance. Beyond that, `m_recently_announced_invs` only tracks things we announced, not things we received, and we rate limit that (as per the static_assert on `INVENTORY_MAX_RECENT_RELAY`).
Delaying MEMPOOL results for 2 minutes seems annoying. But how about just recording the timestamp of the last MEMP
...
💬 ajtowns commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1190528963)
The `3500` figure is calculated based on outbound peers, which already have 2.5x the INV rate of inbound peers. An inbound peer would need to be at 65 or 70 txids per INV message to have the same 1-in-a-million chance of a problem, implying a tx rate of 13 or 14 tx/s. To get to a 1-in-100 chance of a problem, you'd need to hit 90-to-95 txids per INV (and sustain that for ~40 INVs, and hit the 1-in-100 chance of sending those 40 INVs in a 2 minute period). Those seem okay to me.
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1190528963)
The `3500` figure is calculated based on outbound peers, which already have 2.5x the INV rate of inbound peers. An inbound peer would need to be at 65 or 70 txids per INV message to have the same 1-in-a-million chance of a problem, implying a tx rate of 13 or 14 tx/s. To get to a 1-in-100 chance of a problem, you'd need to hit 90-to-95 txids per INV (and sustain that for ~40 INVs, and hit the 1-in-100 chance of sending those 40 INVs in a 2 minute period). Those seem okay to me.
💬 ajtowns commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1190533182)
Just seems like more code to do it that way, since that is just dealing with txids and `CompareDepthAndScore` already has access to the mempool info.
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1190533182)
Just seems like more code to do it that way, since that is just dealing with txids and `CompareDepthAndScore` already has access to the mempool info.
💬 ajtowns commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1190574243)
It's more related to `MAX_PEER_TX_ANNOUNCEMENTS=5000` (we don't want to send more than we would be willing to receive) and `INVENTORY_MAX_RECENT_RELAY=3500` (we don't want to advertise txids and then refuse to relay them because we've forgotten that we advertised them).
(Could make sense to limit it to 90 for inbounds and 45 for outbounds as (if sustained for two minutes) those values would hit roughly a 1-in-100 chance of overflowing `INVENTORY_MAX_RECENT_RELAY`)
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1190574243)
It's more related to `MAX_PEER_TX_ANNOUNCEMENTS=5000` (we don't want to send more than we would be willing to receive) and `INVENTORY_MAX_RECENT_RELAY=3500` (we don't want to advertise txids and then refuse to relay them because we've forgotten that we advertised them).
(Could make sense to limit it to 90 for inbounds and 45 for outbounds as (if sustained for two minutes) those values would hit roughly a 1-in-100 chance of overflowing `INVENTORY_MAX_RECENT_RELAY`)
💬 MarcoFalke commented on issue "make check errors on big endian OpenBSD 7.2":
(https://github.com/bitcoin/bitcoin/issues/26492#issuecomment-1543500303)
> Everything passed when running ./test/util/test_runner.py --verbose .
That means the issue originally reported is fixed and `make check` fully passes?
> ./test/functional/feature_addrman.py --tracerpc
Can you also try with `./test/functional/feature_addrman.py --tracerpc -l DEBUG` and then `./test/functional/combine_logs.py ./folder-tmp-test/foo/bar`?
(https://github.com/bitcoin/bitcoin/issues/26492#issuecomment-1543500303)
> Everything passed when running ./test/util/test_runner.py --verbose .
That means the issue originally reported is fixed and `make check` fully passes?
> ./test/functional/feature_addrman.py --tracerpc
Can you also try with `./test/functional/feature_addrman.py --tracerpc -l DEBUG` and then `./test/functional/combine_logs.py ./folder-tmp-test/foo/bar`?
⚠️ huzhenyuan opened an issue: "One core in CPU usage rate remains at 100% for a long time, causing serious delays in new blocks and forks"
(https://github.com/bitcoin/bitcoin/issues/27623)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
We are the technical team from AntPool. We found that when the Bitcoin wallet node processes the process message recently, the single-threaded CPU 100% causes block parsing delays, resulting in serious technical failures.
Our version number is the latest version 24.0.1。
Not only antpool, we also asked other mining pools and found this problem. This will lead to increasingly serious orpha
...
(https://github.com/bitcoin/bitcoin/issues/27623)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
We are the technical team from AntPool. We found that when the Bitcoin wallet node processes the process message recently, the single-threaded CPU 100% causes block parsing delays, resulting in serious technical failures.
Our version number is the latest version 24.0.1。
Not only antpool, we also asked other mining pools and found this problem. This will lead to increasingly serious orpha
...
💬 darosior commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1543552214)
I'm only a casual lurker on the net_processing side but i was under the impression we were thinking of the BIP35 as permissioned anyways. If we are already trusting the peer that sends use MEMPOOL message, why not just include everything we've got?
A practical counter-argument to this could be - yeah but at this time [around 20% to 30% of reachable nodes](https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1529678174) set `peerbloomfilters` so it's still worth closing an obvious fingerpr
...
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1543552214)
I'm only a casual lurker on the net_processing side but i was under the impression we were thinking of the BIP35 as permissioned anyways. If we are already trusting the peer that sends use MEMPOOL message, why not just include everything we've got?
A practical counter-argument to this could be - yeah but at this time [around 20% to 30% of reachable nodes](https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1529678174) set `peerbloomfilters` so it's still worth closing an obvious fingerpr
...
💬 willcl-ark commented on issue "One core in CPU usage rate remains at 100% for a long time, causing serious delays in new blocks and forks":
(https://github.com/bitcoin/bitcoin/issues/27623#issuecomment-1543579414)
Hello @huzhenyuan, thank you for the report.
There is a possible fix for similar symptoms currently open in https://github.com/bitcoin/bitcoin/pull/27610, but note that this is to alleviate high CPU usage originating from `SendMessages()`, not `ProcessMessages()` as you describe you have found in your case. However, both of these functions are run in the same thread `b-msghand` so they could be related or difficult to discern from one another.
If you are able and willing, you could try che
...
(https://github.com/bitcoin/bitcoin/issues/27623#issuecomment-1543579414)
Hello @huzhenyuan, thank you for the report.
There is a possible fix for similar symptoms currently open in https://github.com/bitcoin/bitcoin/pull/27610, but note that this is to alleviate high CPU usage originating from `SendMessages()`, not `ProcessMessages()` as you describe you have found in your case. However, both of these functions are run in the same thread `b-msghand` so they could be related or difficult to discern from one another.
If you are able and willing, you could try che
...
💬 FelixWeis commented on issue "One core in CPU usage rate remains at 100% for a long time, causing serious delays in new blocks and forks":
(https://github.com/bitcoin/bitcoin/issues/27623#issuecomment-1543649256)
thanks @willcl-ark, can you edit you comment and point to the relevant mitigation PR/issues.
(https://github.com/bitcoin/bitcoin/issues/27623#issuecomment-1543649256)
thanks @willcl-ark, can you edit you comment and point to the relevant mitigation PR/issues.
💬 fanquake commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#issuecomment-1543653619)
cc @instagibbs @TheBlueMatt
(https://github.com/bitcoin/bitcoin/pull/27622#issuecomment-1543653619)
cc @instagibbs @TheBlueMatt
🚀 fanquake merged a pull request: "refactor, kernel: Decouple ArgsManager from blockstorage"
(https://github.com/bitcoin/bitcoin/pull/27125)
(https://github.com/bitcoin/bitcoin/pull/27125)
💬 kouloumos commented on issue "rpc: Allow importing wallets by data instead of by filename":
(https://github.com/bitcoin/bitcoin/issues/27597#issuecomment-1543668223)
I might be missing something here, as I cannot understand how your latest question is related to your initial feature request.
It is common for backups to be in files, hence importing/restoring from a file. I understand the over-the-wire scenario, but if there is no strong use-case for such a feature, it makes it more difficult to gather interest.
Note that `importwallet` and the related `dumpwallet` RPC are only compatible with legacy wallets, and as there is a [proposed timeline for their
...
(https://github.com/bitcoin/bitcoin/issues/27597#issuecomment-1543668223)
I might be missing something here, as I cannot understand how your latest question is related to your initial feature request.
It is common for backups to be in files, hence importing/restoring from a file. I understand the over-the-wire scenario, but if there is no strong use-case for such a feature, it makes it more difficult to gather interest.
Note that `importwallet` and the related `dumpwallet` RPC are only compatible with legacy wallets, and as there is a [proposed timeline for their
...
💬 fanquake commented on pull request "kernel: Remove args, chainparams, chainparamsbase from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27576#issuecomment-1543685914)
```bash
src/common/settings.h seems to be missing the expected include guard:
#ifndef BITCOIN_COMMON_SETTINGS_H
#define BITCOIN_COMMON_SETTINGS_H
...
#endif // BITCOIN_COMMON_SETTINGS_H
```
(https://github.com/bitcoin/bitcoin/pull/27576#issuecomment-1543685914)
```bash
src/common/settings.h seems to be missing the expected include guard:
#ifndef BITCOIN_COMMON_SETTINGS_H
#define BITCOIN_COMMON_SETTINGS_H
...
#endif // BITCOIN_COMMON_SETTINGS_H
```
💬 ajtowns commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1190924614)
> But how about just recording the timestamp of the last MEMPOOL request ...
Patch might look something like...
<details><summary>Details</summary>
```diff
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -295,6 +295,8 @@ struct Peer {
* permitted if the peer has NetPermissionFlags::Mempool or we advertise
* NODE_BLOOM. See BIP35. */
bool m_send_mempool GUARDED_BY(m_tx_inventory_mutex){false};
+ std::chrono::microseconds m_last_memp
...
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1190924614)
> But how about just recording the timestamp of the last MEMPOOL request ...
Patch might look something like...
<details><summary>Details</summary>
```diff
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -295,6 +295,8 @@ struct Peer {
* permitted if the peer has NetPermissionFlags::Mempool or we advertise
* NODE_BLOOM. See BIP35. */
bool m_send_mempool GUARDED_BY(m_tx_inventory_mutex){false};
+ std::chrono::microseconds m_last_memp
...
💬 MarcoFalke commented on issue "rpc: Allow importing wallets by data instead of by filename":
(https://github.com/bitcoin/bitcoin/issues/27597#issuecomment-1543700893)
Passing large data blobs, presumably multi-megabyte raw byte files, encoded as (presumably) hex, wrapped in json, over RPC is likely going to cause more issues than it fixes.
See also https://github.com/bitcoin/bitcoin/issues?q=%22Argument+list+too+long%22:
* GUI https://github.com/bitcoin/bitcoin/issues/17618
* Bash https://github.com/bitcoin/bitcoin/issues/4099
* Python https://github.com/bitcoin/bitcoin/issues/14767#issuecomment-619644381
(https://github.com/bitcoin/bitcoin/issues/27597#issuecomment-1543700893)
Passing large data blobs, presumably multi-megabyte raw byte files, encoded as (presumably) hex, wrapped in json, over RPC is likely going to cause more issues than it fixes.
See also https://github.com/bitcoin/bitcoin/issues?q=%22Argument+list+too+long%22:
* GUI https://github.com/bitcoin/bitcoin/issues/17618
* Bash https://github.com/bitcoin/bitcoin/issues/4099
* Python https://github.com/bitcoin/bitcoin/issues/14767#issuecomment-619644381
💬 jonatack commented on pull request "ci: Run iwyu on all src files":
(https://github.com/bitcoin/bitcoin/pull/27571#issuecomment-1543705947)
Concept ACK. I've been adding sources files to this list while working on each pull.
(https://github.com/bitcoin/bitcoin/pull/27571#issuecomment-1543705947)
Concept ACK. I've been adding sources files to this list while working on each pull.
📝 fanquake opened a pull request: "[23.2] Backports for rc1"
(https://github.com/bitcoin/bitcoin/pull/27624)
Collecting backports for rc1. Currently:
* https://github.com/bitcoin/bitcoin/pull/27608 (not a clean cherry-pick)
(https://github.com/bitcoin/bitcoin/pull/27624)
Collecting backports for rc1. Currently:
* https://github.com/bitcoin/bitcoin/pull/27608 (not a clean cherry-pick)
💬 jonatack commented on pull request "net, refactor: extract Network and BIP155Network logic to node/network":
(https://github.com/bitcoin/bitcoin/pull/27385#discussion_r1190956000)
Good point; removed that commit. Thanks for having a look @fanquake.
(https://github.com/bitcoin/bitcoin/pull/27385#discussion_r1190956000)
Good point; removed that commit. Thanks for having a look @fanquake.
💬 ajtowns commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1190966758)
Okay, other idea: how about just making sure you only send at most 3500 mempool entries younger than 2 minutes, in response to a MEMPOOL message, and add them all to the filter, and don't worry if that causes overflow on results from a previous response?
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1190966758)
Okay, other idea: how about just making sure you only send at most 3500 mempool entries younger than 2 minutes, in response to a MEMPOOL message, and add them all to the filter, and don't worry if that causes overflow on results from a previous response?
💬 willcl-ark commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1543749307)
> i was under the impression we were thinking of the BIP35 as permissioned anyways.
> ...
> For instance BTCPay/NBXplorer needs `whitelist=mempool` iirc.
Just on these points, currently the MEMPOOL message will be responded to _either_ if you have the whitelist permission `mempool` on the peer **or** if you have enabled NODE_BLOOM (with `-peerbloomfilters`). In the latter case I don't think we can accurately describe these as "trusted peers". See the changes in [`4581a68` (#27559)](https://
...
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1543749307)
> i was under the impression we were thinking of the BIP35 as permissioned anyways.
> ...
> For instance BTCPay/NBXplorer needs `whitelist=mempool` iirc.
Just on these points, currently the MEMPOOL message will be responded to _either_ if you have the whitelist permission `mempool` on the peer **or** if you have enabled NODE_BLOOM (with `-peerbloomfilters`). In the latter case I don't think we can accurately describe these as "trusted peers". See the changes in [`4581a68` (#27559)](https://
...