📝 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.

💬 Sjors commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#issuecomment-2208349301)
cc @glozow who added this as part of #26695.

⚠️ Sjors opened an issue: "ci: failure in p2p_node_network_limited.py"
(https://github.com/bitcoin/bitcoin/issues/30389)
The PR seems unrelated, so assuming the failure is spurious.

https://github.com/bitcoin/bitcoin/actions/runs/9789643429/job/27029722411?pr=30356

💬 TheCharlatan commented on pull request "validation: Check if mempool exists before size check in ActivateSnapshot":
(https://github.com/bitcoin/bitcoin/pull/30388#issuecomment-2208359932)
Changed description to drop the bugfix label.

Re 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.

We do the same check on the active chainstate here: https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L4587. So as @maflcko points out, it would be good for consistency's s
...

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#discussion_r1665307304)
It's not polling over RPC.

💬 glozow commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#issuecomment-2208382368)
> Is there a reason why
>
> ```
> BlockAssembler::Options options;
> ApplyArgsManOptions(gArgs, options);
> ```
>
> has to be inside the block assembler class?

It doesn't need to be inside `BlockAssembler`. Prior to #26695, the ctor was using the global `gArgs` to decide on parameters (yuck!). We didn't have the node/*args kernel/*opts conventions at the time and I was pretty happy adding an `ApplyArgsMan` that didn't do that.

> Why not construct them externally and pass a referen
...

✅ fanquake closed an issue: "ci: failure in p2p_node_network_limited.py"
(https://github.com/bitcoin/bitcoin/issues/30389)

💬 fanquake commented on issue "ci: failure in p2p_node_network_limited.py":
(https://github.com/bitcoin/bitcoin/issues/30389#issuecomment-2208412707)
It's the same issue as #29090.

💬 maflcko commented on issue "ci: wallet_listtransactions.py --legacy-wallet failure":
(https://github.com/bitcoin/bitcoin/issues/28411#issuecomment-2208414978)
Duplicate of https://github.com/bitcoin/bitcoin/issues/29090?

✅ fanquake closed an issue: "ci: wallet_listtransactions.py --legacy-wallet failure"
(https://github.com/bitcoin/bitcoin/issues/28411)

💬 dergoegge commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#discussion_r1665352969)
> How is this not polling?

I was reading it as "check every 50ms" but I just noticed that it is also using a condvar, so it'll get notified when `g_best_block` changes.

⚠️ maflcko opened an issue: "ci: ConnectionRefusedError: [WinError 10061] No connection could be made because the target machine actively refused it"
(https://github.com/bitcoin/bitcoin/issues/30390)
Usually they come with a follow-up error of `Unable to connect to bitcoind after 2400s` (or similar).

This is a tracking issue, because all tests are affected.