💬 hebasto commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#issuecomment-1497004515)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/27419#issuecomment-1497004515)
Concept ACK.
💬 ajtowns commented on pull request "validation: Move warningcache to ChainstateManager and rename to m_warningcache":
(https://github.com/bitcoin/bitcoin/pull/27357#issuecomment-1497009569)
ACK 552684976b6df34ce563458f73812e6e494e3b0e
Looking at this, it occurs to me that we're actually being a bit wasteful scanning through ~780k historical headers 29 times (or 2.4M headers 29 times for testnet) every time we startup. Replacing `MinBIP9WarningHeight` with `MinBIP9WarningStartTime` (set it to say 2022-07-01 initially), and using it for `WarningBitsConditionChecker::BeginTime()`, and just bumping it at each release might be worthwhile.
(https://github.com/bitcoin/bitcoin/pull/27357#issuecomment-1497009569)
ACK 552684976b6df34ce563458f73812e6e494e3b0e
Looking at this, it occurs to me that we're actually being a bit wasteful scanning through ~780k historical headers 29 times (or 2.4M headers 29 times for testnet) every time we startup. Replacing `MinBIP9WarningHeight` with `MinBIP9WarningStartTime` (set it to say 2022-07-01 initially), and using it for `WarningBitsConditionChecker::BeginTime()`, and just bumping it at each release might be worthwhile.
💬 S3RK commented on pull request "wallet: coin selection, don't return results that exceed the max allowed weight":
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1158084125)
I think this should be `change_output_size` not `change_spend_size`
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1158084125)
I think this should be `change_output_size` not `change_spend_size`
💬 hebasto commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1158131412)
Add a copyright header?
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1158131412)
Add a copyright header?
💬 Satoshingmx commented on issue "Bitcoin.org/https://www.bitcoin.org/www.bitcoin.org":
(https://github.com/bitcoin/bitcoin/issues/27416#issuecomment-1497096744)
Yes what is your question
(https://github.com/bitcoin/bitcoin/issues/27416#issuecomment-1497096744)
Yes what is your question
💬 josibake commented on pull request "Detect and ignore transactions that were CPFP'd in the fee estimator":
(https://github.com/bitcoin/bitcoin/pull/25380#issuecomment-1497114881)
Concept ACK
> We'd also need historical snapshots of a mempool to do that. If someone has got some and is willing to test, i'd be very interested in seeing the results.
I have historical snapshots and would be happy to help run the test. Although, since running the test is not trivial, I wouldn't make a blocker for the PR.
(https://github.com/bitcoin/bitcoin/pull/25380#issuecomment-1497114881)
Concept ACK
> We'd also need historical snapshots of a mempool to do that. If someone has got some and is willing to test, i'd be very interested in seeing the results.
I have historical snapshots and would be happy to help run the test. Although, since running the test is not trivial, I wouldn't make a blocker for the PR.
💬 hebasto commented on pull request "refactor: Make `CCheckQueue` RAII-styled":
(https://github.com/bitcoin/bitcoin/pull/26762#discussion_r1158209598)
Doesn't the default `memory_order_seq_cst` memory order for `std::atomic<bool> m_request_stop` guarantee the correct behaviour?
(https://github.com/bitcoin/bitcoin/pull/26762#discussion_r1158209598)
Doesn't the default `memory_order_seq_cst` memory order for `std::atomic<bool> m_request_stop` guarantee the correct behaviour?
:lock: fanquake locked an issue: "Bitcoin.org/https://www.bitcoin.org/www.bitcoin.org"
(https://github.com/bitcoin/bitcoin/issues/27416)
(https://github.com/bitcoin/bitcoin/issues/27416)
💬 MarcoFalke commented on pull request "log: Check that the timestamp string is non-empty to avoid undefined behavior":
(https://github.com/bitcoin/bitcoin/pull/27317#issuecomment-1497144933)
lgtm ACK 73f4eb511cf80cf52b78627347727ca02774b731
(https://github.com/bitcoin/bitcoin/pull/27317#issuecomment-1497144933)
lgtm ACK 73f4eb511cf80cf52b78627347727ca02774b731
💬 Sjors commented on pull request "logging, net: add ASN from peers on logs":
(https://github.com/bitcoin/bitcoin/pull/27412#issuecomment-1497151925)
Tested that 0076bed45eb2b42111fa3f4c95181393c685a42e works.
During review I noticed `fLogIPs` which is controlled by `-logips` which is off by default. This settings seems to be ignored in a bunch of places. I don't know what it's original purpose is. If the issue was peer privacy (which #3764 implies), then even though an ASN is also not an IP, we should probably still hide it when `-logips` is false.
(https://github.com/bitcoin/bitcoin/pull/27412#issuecomment-1497151925)
Tested that 0076bed45eb2b42111fa3f4c95181393c685a42e works.
During review I noticed `fLogIPs` which is controlled by `-logips` which is off by default. This settings seems to be ignored in a bunch of places. I don't know what it's original purpose is. If the issue was peer privacy (which #3764 implies), then even though an ASN is also not an IP, we should probably still hide it when `-logips` is false.
💬 vasild commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1497176155)
Right, incoming Tor need special handling. That is already what `CNode::ConnectedThroughNetwork()` does. So it would look like:
```cpp
// For privacy reasons, don't advertise our privacy-network address
// to other networks and don't advertise our other-network address
// to privacy networks.
const Network our_net{entry.first.GetNetwork()};
const Network peers_net{node.ConnectedThroughNetwork()};
if (our_net != peer
...
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1497176155)
Right, incoming Tor need special handling. That is already what `CNode::ConnectedThroughNetwork()` does. So it would look like:
```cpp
// For privacy reasons, don't advertise our privacy-network address
// to other networks and don't advertise our other-network address
// to privacy networks.
const Network our_net{entry.first.GetNetwork()};
const Network peers_net{node.ConnectedThroughNetwork()};
if (our_net != peer
...
💬 ismaelsadeeq commented on pull request "test: add coverage for `-bantime`":
(https://github.com/bitcoin/bitcoin/pull/26604#discussion_r1137878941)
tAck but how about adding another coverage to test the ban remaining time after the bantime elapsed.
`time.sleep(secs)`
`banned = self.nodes[1].listbanned()[0]`
`assert_equal(banned["time_remaining"],0) `
(https://github.com/bitcoin/bitcoin/pull/26604#discussion_r1137878941)
tAck but how about adding another coverage to test the ban remaining time after the bantime elapsed.
`time.sleep(secs)`
`banned = self.nodes[1].listbanned()[0]`
`assert_equal(banned["time_remaining"],0) `
💬 MarcoFalke commented on pull request "ci: Remove second user account":
(https://github.com/bitcoin/bitcoin/pull/27376#issuecomment-1497207639)
> Unfortunately, I've noticed a regression introduced in this PR.
It works for me on latest master locally and on my CI cluster. Please provide exact steps to reproduce, ideally starting from a fresh install of your Linux.
(https://github.com/bitcoin/bitcoin/pull/27376#issuecomment-1497207639)
> Unfortunately, I've noticed a regression introduced in this PR.
It works for me on latest master locally and on my CI cluster. Please provide exact steps to reproduce, ideally starting from a fresh install of your Linux.
💬 MarcoFalke commented on pull request "ci: use clang-16 in tidy task":
(https://github.com/bitcoin/bitcoin/pull/27404#issuecomment-1497232880)
Also https://github.com/bitcoin/bitcoin/pull/25466#discussion_r930891939
(https://github.com/bitcoin/bitcoin/pull/27404#issuecomment-1497232880)
Also https://github.com/bitcoin/bitcoin/pull/25466#discussion_r930891939
📝 ismaelsadeeq opened a pull request: "test: add coverage to rpc_scantxoutset.py "
(https://github.com/bitcoin/bitcoin/pull/27422)
Include a test that checks whether the first argument of scantxoutset RPC call "start" is required.
The rpc call should fail if the "start" argument is not provided.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for
...
(https://github.com/bitcoin/bitcoin/pull/27422)
Include a test that checks whether the first argument of scantxoutset RPC call "start" is required.
The rpc call should fail if the "start" argument is not provided.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for
...
💬 martinus commented on pull request "refactor: Make `CCheckQueue` RAII-styled":
(https://github.com/bitcoin/bitcoin/pull/26762#discussion_r1158326638)
The only thing guaranteed is the order; so `m_request_stop` will be set to `true` before `notify_all` is called. But there is no guarantee where the other threads currently are because there is now no lock anymore. E.g. a worker thread could be currently here:
```cpp
while (queue.empty() && !m_request_stop) {
if (fMaster && nTodo == 0) { // <-------- Worker thread waiting here due to context switch
nTotal--;
bool fRet = fAllOk;
// reset the status for new wo
...
(https://github.com/bitcoin/bitcoin/pull/26762#discussion_r1158326638)
The only thing guaranteed is the order; so `m_request_stop` will be set to `true` before `notify_all` is called. But there is no guarantee where the other threads currently are because there is now no lock anymore. E.g. a worker thread could be currently here:
```cpp
while (queue.empty() && !m_request_stop) {
if (fMaster && nTodo == 0) { // <-------- Worker thread waiting here due to context switch
nTotal--;
bool fRet = fAllOk;
// reset the status for new wo
...
💬 glozow commented on pull request "test: create random and coins utils, add amount helper, dedupe add_coin":
(https://github.com/bitcoin/bitcoin/pull/26940#issuecomment-1497266140)
What was the rationale for 81f5ade2a324167c03c5ce765a26bd42ed652723? test/util/random.h depends on test/util/setup_common because it's using `g_insecure_rand_ctx`, which means setup_common can't use any of the random functions without creating a circular dependency. Was this intentional?
(https://github.com/bitcoin/bitcoin/pull/26940#issuecomment-1497266140)
What was the rationale for 81f5ade2a324167c03c5ce765a26bd42ed652723? test/util/random.h depends on test/util/setup_common because it's using `g_insecure_rand_ctx`, which means setup_common can't use any of the random functions without creating a circular dependency. Was this intentional?
🚀 fanquake merged a pull request: "compat: move (win) S_* defines into bdb"
(https://github.com/bitcoin/bitcoin/pull/26832)
(https://github.com/bitcoin/bitcoin/pull/26832)
💬 hebasto commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#discussion_r1158337331)
Missed EOL?
(https://github.com/bitcoin/bitcoin/pull/27335#discussion_r1158337331)
Missed EOL?
💬 hebasto commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#discussion_r1158336326)
Pinning to the latest available version looks suboptimal as it still forces users to upgrade their vcpkg installation.
OTOH, pinning to the previous one, for example, `2.1.12#7`, could be a better option for an additional reason: no need to change `build_msvc/bitcoin_config.h.in` and `build_msvc/common.init.vcxproj.in`.
(https://github.com/bitcoin/bitcoin/pull/27335#discussion_r1158336326)
Pinning to the latest available version looks suboptimal as it still forces users to upgrade their vcpkg installation.
OTOH, pinning to the previous one, for example, `2.1.12#7`, could be a better option for an additional reason: no need to change `build_msvc/bitcoin_config.h.in` and `build_msvc/common.init.vcxproj.in`.