💬 mzumsande commented on issue "Assertion chainman().ActiveChain()[height] fails on startup on memory-constrained system":
(https://github.com/bitcoin/bitcoin/issues/29422#issuecomment-2207413826)
I could reproduce this, both on 26.0 and master, by simlulating the oom by adding the assert **after** the block index was loaded.
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index fe466fe5aa..352c802576 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2891,6 +2891,7 @@ bool Chainstate::FlushStateToDisk(
if (!m_blockman.WriteBlockIndexDB()) {
return FatalError(m_chainman.GetNotifications(), state, _("Failed to write to bl
...

💬 tdb3 commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1664879365)
Yeah, it would. The thought here is to increase resilience in the code, so that if ai_flags changes we're not hard-coded to 0. Seems like the scope of this `if` condition is intended as "if AI_ADDRCONFIG set" rather than "is only AI_ADDRCONFIG set" (which is now updated in f95fb4a1d826765553f3319ee6f1024094954073). I'm thinking we would want `callgetaddrinfo()` to use similar logic (i.e. keep flags defined except for AI_ADDRCONFIG).

👍 cbergqvist approved a pull request: "Make it possible to disable Tor binds and abort startup on bind failure"
(https://github.com/bitcoin/bitcoin/pull/22729#pullrequestreview-2157640758)
ACK bca346a97056748f1e3fb899f336d56d9fd45a64

>> The current solution means most test nodes in the functional test suite will now bind to an onion port even though the vast majority are not testing Tor

> Only tests that use `bind_to_localhost_only` will do. We have 5 such tests.

We have 5 tests that *disable* `bind_to_localhost_only`, all the others will `bind=127.0.0.1`. Having that will result in your new `has_explicit_bind` being true for the majority of tests, which will lead to your
...

💬 fjahr commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#issuecomment-2207510865)
> > I also removed the mempool and it crashed when reaching validation.cpp:5684. Is it a hard requirement when using assumeutxo that a mempool exists, or is this a bug?
>
> That appears to be a bug. We actually require the mempool to be empty so it not being there should not be a problem. I will open a PR.

This should work https://github.com/fjahr/bitcoin/commit/21530b489eee6b8db4b9af45430d9d3cb43a459c, but on second thought I am not sure if we need this (yet). Conceptually it makes sense
...

📝 theuni opened a pull request: "contrib: use c++ compiler rather than c compiler for binary checks"
(https://github.com/bitcoin/bitcoin/pull/30387)
See discussion here: https://github.com/hebasto/bitcoin/pull/252#discussion_r1664657488

Use CXX/CXXFLAGS rather than CC/CFLAGS to test our actual compiler for binary checks rather than the one we only forward to secp256k1.

👍 tdb3 approved a pull request: "Update manpage descriptions"
(https://github.com/bitcoin/bitcoin/pull/29686#pullrequestreview-2157725238)
re ACK f6171a8f1dabc155c47faf9a67ef87dea436c623
Re-ran actions in previous review https://github.com/bitcoin/bitcoin/pull/29686#pullrequestreview-1956428566

💬 maflcko commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#issuecomment-2208193267)
> I checked if there is anything we could do in the testing setup to make the test faster. With this patchset, the test takes around 1:15 min for 10k runs on my machine. Removing the validation interface from the test setup cuts it down to 1:05 min , if I remove the networking setup, it takes 35 sec to complete.

Nice. Would be good to have a pull request with this, so that it can be reviewed and merged.

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1665187839)
This is already a list initialization, so I don't think clang-tidy can pick up the named args at all. Happy to review a follow-up, if you decide to open one.

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1665189772)
I am happy to review a follow-up, if you decide to open one.

💬 Sjors commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#issuecomment-2208225151)
Rebased after #30324.

💬 Sjors commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1665208121)
This is copied from `GetNextWorkRequired` which doesn't have the `nHeight` helper. The simplification makes sense, or we can add a helper function: https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1654288556

📝 TheCharlatan opened a pull request: "bugfix: Check if mempool exists before asserting in ActivateSnapshot"
(https://github.com/bitcoin/bitcoin/pull/30388)

💬 TheCharlatan commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#issuecomment-2208252561)
Is there a reason why
```
BlockAssembler::Options options;
ApplyArgsManOptions(gArgs, options);
```
has to be inside the block assembler class? Why not construct them externally and pass a reference of the options to the `BlockAssembler`? This seems a bit more efficient, since the options don't have to be constructed and the arguments not read every time a template is retrieved.

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1665234886)
k, thanks, ignore this :)

💬 maflcko commented on pull request "bugfix: Check if mempool exists before size check in ActivateSnapshot":
(https://github.com/bitcoin/bitcoin/pull/30388#issuecomment-2208289115)
It can only be nullptr in unit tests, not in `bitcoind`. However, it seems fine to add this check for consistency with the other code.

lgtm ACK 39ec860eb8b867cce4ed437049abc0e031fd3c95

💬 maflcko commented on pull request "wallet: use LogTrace for walletdb log messages at trace level":
(https://github.com/bitcoin/bitcoin/pull/30355#issuecomment-2208294155)
> Could also change it to `[warn]` when the user request could not be fulfilled?

@ajtowns Did you see this? Seems rfm either way, but maybe address this or reply, so that maintainers don't hold it back on this nit?

💬 Sjors commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#issuecomment-2208298339)
@vasild thanks! At first glance these changes make sense, not sure why it's broken. I left some comments on your commit f0dc62f8ab773ef7cbf44ba7119d08af66506428.

💬 mzumsande commented on pull request "bugfix: Check if mempool exists before size check in ActivateSnapshot":
(https://github.com/bitcoin/bitcoin/pull/30388#issuecomment-2208305841)
I wouldn't call this a "bug" because I think we can assume that the _active_ chainstate (which is checked here, not any chainstate) must have a mempool attached - this is not optional. But if it helps the tests, why not?

💬 stratospher commented on pull request "Test/rpc whitelistdefault test":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1650916093)
nit: this seems implied. I really liked the more descriptive comments in [this file](https://github.com/emilengler/bitcoin/blob/a478f6d6cbce3dc37efb38e4146ddfc56e79b18c/test/functional/rpc_whitelistdefault.py#L37). maybe you could include some form of that?

💬 stratospher commented on pull request "Test/rpc whitelistdefault test":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1665286212)
that's because `with open(os.path.join(get_datadir_path(self.options.tmpdir, 0), "bitcoin.conf"), 'a', encoding='utf8') as f` creates a new bitcoin.conf file inside `bitcoin` but outside `node0` directory. the bitcoin.conf file inside `node0` directory where the actual permissions are read from is unchanged. See[ L65](https://github.com/naiyoma/bitcoin/blob/e02af85abaec67d92e976383777a4b0f2caae4e1/test/functional/rpc_whitelist.py#L65) for example.