💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2844593964)
> Can we maybe just hold off on this for now? The investigation above prompted me to start working on a much larger refactor...

Not sure about that because I have not seen that other refactor. In general I prefer do stuff in smaller, more manageable steps. I guess a further refactor should be possible on top of this PR. I would be happy to review. This PR should make a more solid foundation for further changes in the code.

👍 l0rinc approved a pull request: "validation: write chainstate to disk every hour"
(https://github.com/bitcoin/bitcoin/pull/30611#pullrequestreview-2809508852)
ACK e976bd3045010ee217aa0f2dca4c962aabb789d5

I'm glad to see this moving along, this will help other optimizations as well by eliminating the worst-case scenario of crashing at the end of IBD with an OOM.

I'm fine with merging as it is, I only have nits and minor suggestions - but will glady reack if they're applied.
I've retested it locally by running all tests and doing a few thousand blocks of IBD in debug mode (without any changes and with modified `1min`..`2min` interval to trigger
...

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r2070087705)
we can inline `mNow`:
```suggestion
const bool fPeriodicWrite{mode == FlushStateMode::PERIODIC && NodeClock::now() >= m_next_write};
```

nit: maybe we could add some cleanup commit here which uses brace inits (to avoid all the `=` vs `==` confusion here) and to unify the names of these flag variables

💬 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