💬 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

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2616360178)
```suggestion
* - Query whether a given recipient node has confirmed reception
* - Query whether any transactions are currently in the list
```

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2616365515)
I would support changing this to be more aggressive. In my testing the happy path dominated, but it would be nice to get faster feedback if your tx is no longer valid for whatever reason.
Also, when I tested with lower than default min relay fee, this would have likely gotten my tx successfully sent sooner.

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2616359532)
nit
```suggestion
* - Mark that a given recipient node has confirmed receipt of a transaction
```

💬 bigspider commented on pull request "OP_CHECKCONTRACTVERIFY":
(https://github.com/bitcoin/bitcoin/pull/32080#issuecomment-3649665094)
cc @Sjors

PR to Inquisition created: https://github.com/bitcoin-inquisition/bitcoin/pull/102

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3649711079)
code review diff ACK ebc62086da5ff083000a3aa4eab21fa769908a13


Changes since last ack (`git range-diff f391296b17a755153960e0afc736df37d1d5fb1e...ebc62086da5ff083000a3aa4eab21fa769908a13`):
* to avoid the input-output parameter confusion, the optional proxy output param was reset at the beginning of `CConnman::PrivateBroadcast::PickNetwork`;
* 0.1s sleep in when there are remaining connections to avoid debug log churn;
* guard retries to avoid needless rebroadcasts and possibly being stuc
...

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

💬 rustaceanrob commented on pull request "Make `transaction_indentifier` hex string constructor evaluated at comptime":
(https://github.com/bitcoin/bitcoin/pull/34063#issuecomment-3649770131)
Co-authorship attribution, rebased into single commit, brace initialization throughout the commit, and `constexpr` fix in 5ac35795206d252c9f464e967b84521ddaad38f1

💬 l0rinc commented on pull request "Make `transaction_indentifier` hex string constructor evaluated at comptime":
(https://github.com/bitcoin/bitcoin/pull/34063#issuecomment-3649780755)
ACK 5ac35795206d252c9f464e967b84521ddaad38f1

I'm fine with this version and with @ajtowns's [suggestion](https://github.com/bitcoin/bitcoin/pull/34063#discussion_r2615793433) as well - let's see what others think.

💬 ajtowns commented on pull request "net: Waste less time in socket handling":
(https://github.com/bitcoin/bitcoin/pull/34025#discussion_r2616553073)
What I was seeing was that the duration constructor was noexcept in practice, but time_point has an explicit constructor that wouldn't throw but wasn't declared as noexcept, which was then rejected by atomic...

Stackoverflow answer about it: https://stackoverflow.com/questions/22701617/should-constructors-on-stdchrono-time-point-be-noexcept-or-why-arent

Ah, it looks like it changed between C++17 and C++20, going from `atomic() noexcept = default;` to `constexpr atomic() noexcept(is_nothrow
...