💬 stickies-v commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1566042673)
Sorry, I think it must have regressed with [this update in the next line](https://github.com/bitcoin/bitcoin/compare/2ef71c73582be554e565ada3f8a6ca77c2cd79f1..5362db10ab6d1d6697807c8f8494a2fc5bf41ff8#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R3687) - will fix if I have to retouch.

💬 alfonsoromanz commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2057231800)
Concept ACK

💬 instagibbs commented on pull request "fuzz: explicitly cap the vsize of RBFs for diagram checks":
(https://github.com/bitcoin/bitcoin/pull/29879#issuecomment-2057240637)
> there is no strict need to tie it to standard sizes.

Pretty sure we can overflow if we allow ~1MvB transactions, as `10,000 * 1,000,000 > 2^31`

Made a more direct limiting attempt. I will squash if it is preferred

🤔 BrandonOdiwuor reviewed a pull request: "Add Signet launch shortcut for Windows"
(https://github.com/bitcoin/bitcoin/pull/26334#pullrequestreview-2001547908)
Concept ACK

📝 fanquake opened a pull request: "depends: build FreeType with CMake"
(https://github.com/bitcoin/bitcoin/pull/29880)
Bumps FreeType from [`2.11.0`](https://sourceforge.net/projects/freetype/files/freetype2/2.11.0/) to [`2.13.2`](https://sourceforge.net/projects/freetype/files/freetype2/2.13.2/) (currently untested/reviewed).
Switches Freetype to be built with CMake.

🚀 fanquake merged a pull request: "[27.x] More backports and finalize"
(https://github.com/bitcoin/bitcoin/pull/29780)

💬 theStack commented on pull request "test: p2p: add test for rejected tx request logic (`m_recent_rejects` filter)":
(https://github.com/bitcoin/bitcoin/pull/29827#discussion_r1566089752)
> I'll punt to future work(TM) but I was wondering if we could just generate an "ephemeral" miniwallet inside `fill_mempool` if the resulting utxos from filling the mempool aren't necessary for the rest of the test?

Hmm for that purpose I think it would be nice if different MiniWallet instances create different output scripts, to avoid confusion. Currently, MiniWallet instances for the same output type (e.g. the default `ADDRESS_OP_TRUE`) always result in the same UTXO pool (at least, after t
...

💬 stickies-v commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1566121275)
We `#include <util/time.h>` which has `using namespace std::chrono_literals;` which is why it compiles. I thought we would prefer (IWYU-style) to be explicit about used namespaces, but upon further investigation the purpose of #20602 seems to have been to _avoid_ that, so I'll remove this if I re-touch.

💬 pinheadmz commented on issue "./bitcoin.conf file should not cause confusion with ./datadir/bitcoin.conf":
(https://github.com/bitcoin/bitcoin/issues/29139#issuecomment-2057375048)
> IMO folks looking for an example conf would also be more likely to look for and find it in `share/`.

agreed. im testing your commit in a guix build locally, will ack that PR

💬 stickies-v commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1566154090)
I generally tend to prefer to doxygen-ify documentation, but in this case I'm not sure the extra line adds anything over the return type and function name, so then is it just unnecessary boilerplate that makes the documentation less instead of more clear? No strong view either way, so will leave this open.

📝 fanquake opened a pull request: "guix: use GCC 13 to builds releases"
(https://github.com/bitcoin/bitcoin/pull/29881)
Based on #29828 and #29846 (Windows cleansup, please review first).

Also requires a bump to the Guix time-machine, to pull in the relevant Windows fixups:
* https://git.savannah.gnu.org/cgit/guix.git/commit/?id=ffd1d0c2d486b9de35706ca73a4e89177114e4de
* https://git.savannah.gnu.org/cgit/guix.git/commit/?id=97bad5e1947a07d96f40205bec1e91463e7bf014

Otherwise, is enough to switch release builds to using GCC 13.2.0: https://gcc.gnu.org/gcc-13/.

Does not solve the cross-arch non-determinis
...

💬 achow101 commented on pull request "bumpfee: ignore WALLET_INCREMENTAL_RELAY_FEE when user specifies fee_rate":
(https://github.com/bitcoin/bitcoin/pull/27969#issuecomment-2057413771)
ACK f58beabe754363cb7d5b24032fd392654b9514ac

💬 naiyoma commented on pull request "test: Refactor fee calculation to remove satoshi_round function":
(https://github.com/bitcoin/bitcoin/pull/29566#issuecomment-2057426877)
> tACK [b7a4a81](https://github.com/bitcoin/bitcoin/pull/29566/commits/b7a4a8118b43ebd2ab1b64247990ff49d6f19d26)
>
> Make successful, all functional tests successful.
>
> > To address this, the satoshi_round function is removed, and Decimal values are utilized in its place.
>
> 1. As per the second commit, the `satoshi_round` is back, let's update the PR description to reflect that because it will be picked up by the bot on merge?
> 2. Should we consider squashing the first 2 commits i
...

🤔 stickies-v reviewed a pull request: "kernel: De-globalize fReindex"
(https://github.com/bitcoin/bitcoin/pull/29817#pullrequestreview-2001088868)
Approach ACK d9bcbbf2293ef427b37eefca30f074be5eeeca26 and code LGTM.

---

Some thoughts I have from reviewing this PR, but that are orthogonal to it:

It seems like we have 4 places where we store information about reindexing being required or being in progress:
1. `BlockTreeDB` (through `WriteReindexing()` and `ReadReindexing()`)
1. `BlockManager::m_reindex` (previously `fReindex`)
1. `BlockManagerOpts::reindex`
1. `ChainstateLoadOptions::reindex`

1\. and 2. are updated based on r
...

💬 stickies-v commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1565782441)
To avoid defining the default value twice (default member init in `BlockManagerOpts`), perhaps better to use:

```suggestion
if (auto value{args.GetBoolArg("-reindex")}) opts.reindex = *value;
```

💬 laanwj commented on pull request "tracing: cast block_connected duration to µs":
(https://github.com/bitcoin/bitcoin/pull/29877#issuecomment-2057427995)
Code review ACK ca4eaa5777888292bbdef3ae8d45af6a407df5df

💬 pinheadmz commented on pull request "netbase: remove unnecessary log message":
(https://github.com/bitcoin/bitcoin/pull/29649#issuecomment-2057446510)
> far less frequent

Those logs should only print when the users local proxy is unreachable, which should be less frequent and also a bit more justified. However it is also spammy in that case. I ran with `-proxy=unix:/path/to/tor/socket` and then after a minute, shut down the Tor daemon. Do you think this should be cleaned up as well?

```
2024-04-15T17:22:45Z connect() to 127.0.0.1:7656 failed after wait: Connection refused (61)
2024-04-15T17:22:47Z [error] Error while reading proxy resp
...

💬 achow101 commented on pull request "script/sign: avoid duplicated signature verification after signing (+introduce signing benchmarks)":
(https://github.com/bitcoin/bitcoin/pull/28923#issuecomment-2057453786)
ACK 3a3ccf00f0bb6b6450dcf859982cdb40bb06f475

💬 pinheadmz commented on pull request "netbase: remove unnecessary log message":
(https://github.com/bitcoin/bitcoin/pull/29649#issuecomment-2057454163)
Although actually the unix sockets PR didnt change this behavior much. If you start v26 with a black hole like `-proxy=127.0.0.1:9999` you get similar spam:

```
2024-04-15T17:28:12Z connect() to 127.0.0.1:7656 failed after wait: Connection refused (61)
2024-04-15T17:28:12Z connect() to 127.0.0.1:9999 failed after wait: Connection refused (61)
2024-04-15T17:28:13Z connect() to 127.0.0.1:7656 failed after wait: Connection refused (61)
2024-04-15T17:28:13Z connect() to 127.0.0.1:9050 failed
...

👍 stickies-v approved a pull request: "test: Add missing Assert(mock_time_in >= 0s) to SetMockTime"
(https://github.com/bitcoin/bitcoin/pull/29872#pullrequestreview-2001765599)
ACK fae0db555c12dca75fb09e5fa7bbabdf39b8c1df