💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r2070109326)
nit: if you think the second part needs explanation (i.e. we're scheduling the first periodic write regardless of whether that call wrote or not, but if we write after that, we'll bump the next scheduled write to avoid flushing too often), we could add a comment or extract to a meaningful constant:
```suggestion
const bool next_write_set{m_next_write < NodeClock::time_point::max()};
if (!next_write_set || should_write) {
```
or
```suggestion
const bool first_flush{
...

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r2070099740)
nit: to signal the real boundaries of the random range (i.e. current time + random wait in a strictly positive interval), we could extract the current time outside, e.g.:
```C++
m_next_write = NodeClock::now() + FastRandomContext().rand_uniform_delay(DATABASE_WRITE_INTERVAL_MIN, range);
```

or if you're feeling adventurous:
```C++
m_next_write = NodeClock::now() +
DATABASE_WRITE_INTERVAL_MIN +
FastRandomContext().randrange<std::chrono::minutes>(DATABASE_WR
...

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r2070123523)
The move doesn't seem to affect the evaluation of the very first `fPeriodicWrite` call, since `AcceptBlock` (having `FlushStateMode::NONE`) is always called before `ActivateBestChain` (having`FlushStateMode::PERIODIC`), so `m_next_write` will already be set (because of `m_next_write == NodeClock::time_point::max()`) by the time we get to evaluate whether the next period write is due (i.e `NodeClock::now() >= m_next_write`).

------

Note that before this code move, the writes happened "rough
...

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r2070162874)
The scenarios have become quite complex, would be cool to have a debug log here for why the write was triggered in the first place.

👋 laanwj's pull request is ready for review: "common: Close non-std fds before exec in RunCommandJSON"
(https://github.com/bitcoin/bitcoin/pull/32343)

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r2070174653)
> before this code move, the writes happened "roughly an hour after start or the last write was scheduled", but after this change "roughly an hour after start or the last write finished"

I believe that was the intention of this change (to me at least, not sure if @achow101 had something else in mind).

Very long flushes on slow disks could take over an hour (hopefully not anymore when this is merged), so that would schedule the next flush immediately. Lengthy flushes would schedule next flushes
...

💬 l0rinc commented on pull request "policy: allow more than one OP_RETURN outputs per tx":
(https://github.com/bitcoin/bitcoin/pull/32381#issuecomment-2844690315)
These changes caught many people off guard, and the perceived urgency understandably triggered strong reactions.
For proposals this contentious, it helps to start with an educational phase: GitHub issues, blog posts, podcasts, conference talks, and perhaps even a draft BIP, so the rationale is clear and both positions are properly steel-manned before opening a PR (ideally only after the subject has become mundane).
Without that groundwork, it is easy to understand why some view the process as
...

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r2070180650)
Yes, agree, just noted that this introduces a drift - which, as you also pointed out, makes sense.

💬 Sjors commented on pull request "policy: allow more than one OP_RETURN outputs per tx":
(https://github.com/bitcoin/bitcoin/pull/32381#issuecomment-2844699289)
@l0rinc there's a similar proposal on the mailinglist: https://gnusha.org/pi/bitcoindev/ni6KOcP4nfh6qeB7hEprfzZJuOtBTK6g7rv1L3IRa2TIXq3XUX-7MWGPCHbBATaxOdgbzEQsZmJhId5a4BkGmRabCxSsN24JGEiR4r67D9Y=@pm.me/

💬 Sjors commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2844704124)
@laanwj can you add some testing hints to the PR description? E.g. is there a way to deliberately trigger the potential bug that this fixes?

🤔 janb84 reviewed a pull request: "mining: rename gbt_force and gbt_force_name"
(https://github.com/bitcoin/bitcoin/pull/32386#pullrequestreview-2809667848)
ACK [0750249](https://github.com/bitcoin/bitcoin/commit/0750249289c092fc8e2e29669fec73a58b873767)

The rename of the variables results in less confusion of the intention of the code. And the extra comment explaining the workings of the ! prefix helps to clarify intentions/code even further.

📝 vasild opened a pull request: "net: make m_nodes_mutex non-recursive"
(https://github.com/bitcoin/bitcoin/pull/32394)
The only case of a recursive lock was a nested `ForNode()` call to trim
the size of `lNodesAnnouncingHeaderAndIDs` to `<= 3`. This need not be
nested, so take it out.

Before:
```
// we know we will push one new element at the back
if (size >= 3) pop from front
push at the back
```

After:
```
// maybe push at the back
if (size > 3) pop from the front
```

`lNodesAnnouncingHeaderAndIDs` is protected by `cs_main` which is locked
during the entire operation.

Partially resolves
...

💬 laanwj commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2844721484)
> [@laanwj can you add some testing hints to the PR description? E.g. is there a way to deliberately trigger the potential bug that this fixes?

Sure: The simplest way to see if everything properly gets closed is to list `/proc/self/fd` at the beginning in the signer, as in https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2830252505 .

Alternatively, if you really want to create a bug, you could actively try to mess around with file descriptors that are already open at the start o
...

💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#issuecomment-2844725476)
Made `m_nodes_mutex` non-recursive in https://github.com/bitcoin/bitcoin/pull/32394 which is this PR + one more commit.

💬 saikiran57 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2070201366)
Could you please check updated test cases.

💬 willcl-ark commented on issue "test: interface_usdt_net.py failure under --valgrind":
(https://github.com/bitcoin/bitcoin/issues/32374#issuecomment-2844745010)
I was not able to get this to reproduce using any of the sanitizers.

Connecting with `gdb`, shows all pointers and variables as valid at the callsite. I agree that `ConnectionTypeAsString` seems the natural culprit as it returns a temporary string, but even patching that to return `const *char`,still saw `SIGTRAP` raised.

Stepping through the tracepoint saw it execute without error, so it does seem to be a race/lifetime issue, but not really too sure where else to go from here.

💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2844755692)
`15cc989538...8b93e0894f`: fix CI clang-tidy

💬 dergoegge commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2844766695)
ACK 47e713ea5a96d3bb2ddd64c8a87607e5ccea8c72

🤔 sipa reviewed a pull request: "crypto: Use secure_allocator for `AES256_ctx`"
(https://github.com/bitcoin/bitcoin/pull/31774#pullrequestreview-2809732175)
utACK 591764f6170d6f74d7eebb5fec1cbf5b912098a4, just coding style nits

💬 sipa commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#discussion_r2070233390)
Coding style nit: `} else {`.