💬 fanquake commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1540310529)
> done
We are likely going to defer mergeing this for the moment, as it conflicts with the next two kernel PRs, and assumeutxo. Up to you, if you'd like to keep it rebased after each merge, in the interim.
(https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1540310529)
> done
We are likely going to defer mergeing this for the moment, as it conflicts with the next two kernel PRs, and assumeutxo. Up to you, if you'd like to keep it rebased after each merge, in the interim.
💬 sr-gi 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_r1188745852)
Do you mean keep adding what you're interested in into the filter so it triggers an `INV` from the node and, therefore, we can request it (or, similarly, keep resetting the filter and just add the one single transaction that we want to probe)? If so, isn't that equivalent to having no filter?
I think in both of the aforementioned cases (which boil down to the same logic if I'm not mistaken) the patch prevents the attacker from requesting unannounced transactions straight-away, they will have
...
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1188745852)
Do you mean keep adding what you're interested in into the filter so it triggers an `INV` from the node and, therefore, we can request it (or, similarly, keep resetting the filter and just add the one single transaction that we want to probe)? If so, isn't that equivalent to having no filter?
I think in both of the aforementioned cases (which boil down to the same logic if I'm not mistaken) the patch prevents the attacker from requesting unannounced transactions straight-away, they will have
...
💬 gits7r commented on issue "CPU DoS on mainnet in debug mode":
(https://github.com/bitcoin/bitcoin/issues/27586#issuecomment-1540339751)
This is true for two full nodes that run with `pruning` off, `peerbloomfilters` off, `maxmempool` set to 2048 and `maxconnections` set to 500, where both have an average between 170 and 225 active peers.
Both use fairly reasonable CPUs and are at 100% CPU usage and a lot of RAM (like 8GB only bitcoind). For reference:
"connections": 222,
"connections_in": 212,
"connections_out": 10,
This is true for two full nodes that run with `pruning` off, `peerbloomfilters` off, `maxmempool` set to 2048 and `maxconnections` set to 500, where both have an average between 170 and 225 active peers.
Both use fairly reasonable CPUs and are at 100% CPU usage and a lot of RAM (like 8GB only bitcoind). For reference:
"connections": 222,
"connections_in": 212,
"connections_out": 10,
![bitcoind-res](https://github.com/bitcoin/bitcoin/assets/6549887/c0c43907-ca26-4af7-bedc-4058f0
...
💬 MarcoFalke 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_r1188763337)
`m_last_mempool_req` is now unused and can be removed completely? Also, it might be good to not touch the `return` here when there is no reason, to avoid code churn and review burden.
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1188763337)
`m_last_mempool_req` is now unused and can be removed completely? Also, it might be good to not touch the `return` here when there is no reason, to avoid code churn and review burden.
💬 MarcoFalke commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1188776917)
nit in d7eef9f49a827e96337febcf20a22b392dc7b51e: Can use `chaiman()`?
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1188776917)
nit in d7eef9f49a827e96337febcf20a22b392dc7b51e: Can use `chaiman()`?
💬 fanquake commented on pull request "ci: Run iwyu on all src files":
(https://github.com/bitcoin/bitcoin/pull/27571#issuecomment-1540384552)
Concept ACK on removing the continual manual updating of this list
(https://github.com/bitcoin/bitcoin/pull/27571#issuecomment-1540384552)
Concept ACK on removing the continual manual updating of this list
💬 fanquake commented on pull request "Remove now-unnecessary poll, fcntl includes from net(base).cpp":
(https://github.com/bitcoin/bitcoin/pull/27530#issuecomment-1540387236)
The scope of this has expanded, and it now conflicts with a number of things, that are much higher priority. You could wait until all the kernel / assumeutxo changes are merged, (and maybe also 27571, to remove the need to update the list), or, revert back to the original non-conflicting change, and that could probably be merged. Otherwise, I think this is likely to just sit for some time, and mostly generate merge-conflict noise in other drahtbot/other PRs.
(https://github.com/bitcoin/bitcoin/pull/27530#issuecomment-1540387236)
The scope of this has expanded, and it now conflicts with a number of things, that are much higher priority. You could wait until all the kernel / assumeutxo changes are merged, (and maybe also 27571, to remove the need to update the list), or, revert back to the original non-conflicting change, and that could probably be merged. Otherwise, I think this is likely to just sit for some time, and mostly generate merge-conflict noise in other drahtbot/other PRs.
💬 fanquake commented on pull request "net, refactor: extract Network and BIP155Network logic to node/network":
(https://github.com/bitcoin/bitcoin/pull/27385#discussion_r1188780365)
In 9bcbbc68682c1a92316d6a2dd8142839c58fe247 (fuzz, refactor: hoist net_len_map in addrman fuzzer to constant): Isn't this already a constant? Seems to just be moving the code out of the function where it's used?
(https://github.com/bitcoin/bitcoin/pull/27385#discussion_r1188780365)
In 9bcbbc68682c1a92316d6a2dd8142839c58fe247 (fuzz, refactor: hoist net_len_map in addrman fuzzer to constant): Isn't this already a constant? Seems to just be moving the code out of the function where it's used?
💬 jamesob commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1540397377)
Rebased.
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1540397377)
Rebased.
🤔 glozow reviewed a pull request: "net processing: avoid serving non-announced txs as a result of a MEMPOOL message"
(https://github.com/bitcoin/bitcoin/pull/27602#pullrequestreview-1418933543)
Concept ACK
The issue on master that I believe this addresses is being able to learn exactly when a transaction enters your mempool. You send `mempool` and get the response, then put out the tx, then `getdata getdata getdata getdata...` until the node tells you the tx instead of `notfound`. Also in general I think it's quite sensible to only serve transactions you announced.
(https://github.com/bitcoin/bitcoin/pull/27602#pullrequestreview-1418933543)
Concept ACK
The issue on master that I believe this addresses is being able to learn exactly when a transaction enters your mempool. You send `mempool` and get the response, then put out the tx, then `getdata getdata getdata getdata...` until the node tells you the tx instead of `notfound`. Also in general I think it's quite sensible to only serve transactions you announced.
💬 fanquake commented on pull request "refactor: Replace global find_value function with UniValue::find_value method":
(https://github.com/bitcoin/bitcoin/pull/27605#issuecomment-1540432168)
With fa0d180f48d4e5253f58aced32768369f963d1c7 & `gcc (GCC) 13.1.1 20230426 (Red Hat 13.1.1-1)`, fixes all issues from #26926 except for:
```bash
test/interfaces_tests.cpp: In member function ‘void interfaces_tests::findCommonAncestor::test_method()’:
test/interfaces_tests.cpp:101:19: warning: possibly dangling reference to a temporary [-Wdangling-reference]
101 | const CChain& active = WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return Assert(m_node.chainman)->ActiveChain());
...
(https://github.com/bitcoin/bitcoin/pull/27605#issuecomment-1540432168)
With fa0d180f48d4e5253f58aced32768369f963d1c7 & `gcc (GCC) 13.1.1 20230426 (Red Hat 13.1.1-1)`, fixes all issues from #26926 except for:
```bash
test/interfaces_tests.cpp: In member function ‘void interfaces_tests::findCommonAncestor::test_method()’:
test/interfaces_tests.cpp:101:19: warning: possibly dangling reference to a temporary [-Wdangling-reference]
101 | const CChain& active = WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return Assert(m_node.chainman)->ActiveChain());
...
💬 glozow commented on issue "CPU DoS on mainnet in debug mode":
(https://github.com/bitcoin/bitcoin/issues/27586#issuecomment-1540433286)
> This is true for 2 (two) full nodes that run with pruning off, peerbloomfilters off, maxmempool set to 2048 and maxconnections set to 500, where both have an average between 170 and 225 active peers.
This particular case seems like you have configured bitcoind to do more than your cpu can handle during higher-traffic times. Consider reducing maxconnections for now. And perhaps shrink maxmempool - may I ask why you are using 2048?
(https://github.com/bitcoin/bitcoin/issues/27586#issuecomment-1540433286)
> This is true for 2 (two) full nodes that run with pruning off, peerbloomfilters off, maxmempool set to 2048 and maxconnections set to 500, where both have an average between 170 and 225 active peers.
This particular case seems like you have configured bitcoind to do more than your cpu can handle during higher-traffic times. Consider reducing maxconnections for now. And perhaps shrink maxmempool - may I ask why you are using 2048?
💬 MarcoFalke commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188847042)
Well, I mean that the `default` case can now be skipped. See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures section `enum class` - `default:`
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188847042)
Well, I mean that the `default` case can now be skipped. See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures section `enum class` - `default:`
💬 MarcoFalke commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188847896)
(Otherwise, if a new chain type is added, it will silently map to `main`)
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188847896)
(Otherwise, if a new chain type is added, it will silently map to `main`)
💬 hebasto commented on pull request "bench: Benchmark all `SHA256` implementations that are available on the system":
(https://github.com/bitcoin/bitcoin/pull/27598#issuecomment-1540490305)
Reworked according to @MarcoFalke's [suggestion](https://github.com/bitcoin/bitcoin/pull/27598#issuecomment-1538554964).
Friendly ping @martinus :)
(https://github.com/bitcoin/bitcoin/pull/27598#issuecomment-1540490305)
Reworked according to @MarcoFalke's [suggestion](https://github.com/bitcoin/bitcoin/pull/27598#issuecomment-1538554964).
Friendly ping @martinus :)
💬 MarcoFalke commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188851857)
What is the point of adding dead code over `assert(false)`? Also, the docstring is now wrong. See also https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188847042
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188851857)
What is the point of adding dead code over `assert(false)`? Also, the docstring is now wrong. See also https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188847042
💬 MarcoFalke commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188852707)
What is the point of adding dead code over `assert(false)`? Also, the docstring is now wrong. See also https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188847042
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188852707)
What is the point of adding dead code over `assert(false)`? Also, the docstring is now wrong. See also https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188847042
💬 MarcoFalke commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#issuecomment-1540528281)
See also https://github.com/bitcoin/bitcoin/pull/14309 , where I added the ChainType enum class.
(https://github.com/bitcoin/bitcoin/pull/27491#issuecomment-1540528281)
See also https://github.com/bitcoin/bitcoin/pull/14309 , where I added the ChainType enum class.
💬 st3b1t commented on pull request "rpc: append rpcauth.py hash in config and show pass":
(https://github.com/bitcoin/bitcoin/pull/27588#issuecomment-1540536920)
> From CI:
>
> ```shell
> 102/264 -
> rpc_users.py
> failed, Duration: 1 s
> stdout:
> 2023-05-06T19:48:12.787000Z TestFramework (INFO): PRNG seed is: 4992424049312590578
> 2023-05-06T19:48:12.806000Z TestFramework (INFO): Initializing test directory /tmp/cirrus-ci-build/ci/scratch/test_runner/test_runner_₿_🏃_20230506_193750/rpc_users_159
> 2023-05-06T19:48:13.197000Z TestFramework (ERROR): Unexpected exception caught during testing
> Traceback (most recent call last):
> File "/
...
(https://github.com/bitcoin/bitcoin/pull/27588#issuecomment-1540536920)
> From CI:
>
> ```shell
> 102/264 -
> rpc_users.py
> failed, Duration: 1 s
> stdout:
> 2023-05-06T19:48:12.787000Z TestFramework (INFO): PRNG seed is: 4992424049312590578
> 2023-05-06T19:48:12.806000Z TestFramework (INFO): Initializing test directory /tmp/cirrus-ci-build/ci/scratch/test_runner/test_runner_₿_🏃_20230506_193750/rpc_users_159
> 2023-05-06T19:48:13.197000Z TestFramework (ERROR): Unexpected exception caught during testing
> Traceback (most recent call last):
> File "/
...
👍 brunoerg approved a pull request: "add ryanofsky to trusted-keys"
(https://github.com/bitcoin/bitcoin/pull/27604#pullrequestreview-1419107706)
ACK 59ebee3fb4181baf20fab263cf1b587ece1bd5e2
Strongly agree on adding @ryanofsky on it.
(https://github.com/bitcoin/bitcoin/pull/27604#pullrequestreview-1419107706)
ACK 59ebee3fb4181baf20fab263cf1b587ece1bd5e2
Strongly agree on adding @ryanofsky on it.