💬 Ataraxia009 commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2616086274)
doc string can be changed. No risk to keep it but also no point to add it imo. Can just be a normal function that only rate limits >= Info Level and only used within this file.

But just move forward its fine

💬 w0xlt commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2616151939)
> can this result in an infinite loop or disconnect/reconnects?

From what I understand, yes. That's the point.

💬 maflcko commented on pull request "net: Waste less time in socket handling":
(https://github.com/bitcoin/bitcoin/pull/34025#discussion_r2616285391)
I may be missing something obvious. https://godbolt.org/z/8GM97aYhs seems to compile fine. And https://eel.is/c++draft/time.duration.cons doesn't mention `noexcept`. Can you give a minimal reproducer, or link to the std docs?

💬 yuvicc commented on pull request "validation: Remove min_pow_checked arg in ProcessNewBlockHeaders":
(https://github.com/bitcoin/bitcoin/pull/34022#issuecomment-3649377448)
ACK 8ca9997e48bb2067ea83fabb1c640af72178f97c

💬 maflcko commented on pull request "Make `transaction_indentifier` hex string constructor evaluated at comptime":
(https://github.com/bitcoin/bitcoin/pull/34063#discussion_r2616304495)
> Had a similar attempt in [#31991 (comment)](https://github.com/bitcoin/bitcoin/pull/31991#discussion_r1995691590) - what do you think @maflcko?

Either should be fine. I just think there is no need to provide the exact same functionality via two ways. That seems confusing.

💬 stickies-v commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#issuecomment-3649442978)
re-ACK fa8a5d215c5a81a7282fd5dd1098f9d3fa40e5db

👍 pablomartin4btc approved a pull request: "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency"
(https://github.com/bitcoin/bitcoin/pull/26988#pullrequestreview-3574392235)
re-ACK b3046cca7182f3399a221757318d24e203092301

💬 maflcko commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2616320977)
thx, done in all commits

👍 pablomartin4btc approved a pull request: "log: Remove brittle and confusing LogPrintLevel"
(https://github.com/bitcoin/bitcoin/pull/34051#pullrequestreview-3574394123)
re-ACK fa8a5d215c5a81a7282fd5dd1098f9d3fa40e5db

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2616332760)
I don't think so - the function does not read it, only sets it. However, when the proxy is not needed, the function would leave it untouched. The only caller passes empty optional, so it is fine, but I changed it to always set it from inside the function - either to empty optional (if proxy is not needed) or to a proxy.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2616334277)
There is a sleep of 500ms just below the log.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2616334923)
Changed, thanks!

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2616338783)
> should just check if there are any txs in the private broadcast queue

Done. Added a new method `HavePendingTransactions()` to achieve that.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3649487501)
`f391296b17...ebc62086da`: address suggestions and use addresses in the functional test such that they do not cause collisions when added to addrman.

💬 pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2616340421)
yeah I'm imagining a case where log gets printed every 500ms, but it is unlikely to get printed even once because I think addrman can be assumed to be clean already

💬 musaHaruna commented on pull request "fees: enable `CBlockPolicyEstimator` return sub 1 sat/vb fee rate estimates":
(https://github.com/bitcoin/bitcoin/pull/33199#issuecomment-3649524413)
Concept ACK (code review up to [b767897](https://github.com/bitcoin/bitcoin/pull/33199/commits/b76789787c43802aaf1207813792ea49ecf18f55)

I’m new to fee estimation, but I was able to understand this PR because it is fairly straightforward.

The idea is to make `DEFAULT_MIN_RELAY_TX_FEE` i.e the minimum feerate a node is willing to relay and accept into its mempool by default equal to `MIN_BUCKET_FEERATE`, which is used by the fee estimator (`CBlockPolicyEstimator`).

By default, if a trans
...

💬 musaHaruna commented on pull request "fees: enable `CBlockPolicyEstimator` return sub 1 sat/vb fee rate estimates":
(https://github.com/bitcoin/bitcoin/pull/33199#discussion_r2616362899)
nit: feel free to ignore.
# We will now mine "numblocks" blocks, generating on average 100 transactions between each block
It makes it easier to read than the previous

💬 musaHaruna commented on pull request "fees: enable `CBlockPolicyEstimator` return sub 1 sat/vb fee rate estimates":
(https://github.com/bitcoin/bitcoin/pull/33199#discussion_r2616364515)
nit: feel free to ignore
```suggestion
# We will now mine "numblocks" blocks, generating on average 100 transactions between each block
```
I think makes it easier to read

💬 Mardiza commented on something "":
(https://github.com/bitcoin/bitcoin/commit/691161d419fe3d82d7a49b511ef80e2b24332aac#r172705754)
Okay

💬 Mardiza commented on something "":
(https://github.com/bitcoin/bitcoin/commit/691161d419fe3d82d7a49b511ef80e2b24332aac#r172705761)
Access my coinbase transaction

👍 andrewtoth approved a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3574423265)
re-ACK ebc62086da5ff083000a3aa4eab21fa769908a13

Changes since last review:
- `proxy` out parameter is `reset` in `PickNetwork`, in case a non-`nullopt` is passed in.
- sleeps for 100ms before retrying a failed connection
- adds `HavePendingTransactions` and uses it to check whether to open a new connection when finalizing a node
- reworded a comment
- reordered some addresses in the functional test