💬 stickies-v commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1266868296)
I agree that we shouldn't have it in both places. No view on what's better (and if we want to move it to opts at all). If no one has a strong view, probably best to minimize churn and leave it as is?

💬 stickies-v commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1266812251)
nit: I think we try to avoid C-style casts?

```suggestion
static_cast<uint32_t>(std::max(int64_t{0}, argsman.GetIntArg("-maxorphantx", DEFAULT_MAX_ORPHAN_TRANSACTIONS)));
```

💬 stickies-v commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1266861948)
Maybe worth introducing move semantics here?

<details>
<summary>git diff on </summary>

```diff
diff --git a/src/init.cpp b/src/init.cpp
index 923da24a7..0b276d7cd 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1540,15 +1540,17 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
ChainstateManager& chainman = *Assert(node.chainman);


- PeerManager::Options peerman_opts;
- peerman_opts.banman = node.banman.get();
- peerman_opts.ig
...

💬 stickies-v commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1266787910)
I think the entire diff in this file can be removed on this commit?

💬 stickies-v commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1266833922)
I think you now get to remove `#include <common/args.h>` 🥳

🤔 stickies-v reviewed a pull request: "net processing, refactor: Decouple PeerManager from gArgs"
(https://github.com/bitcoin/bitcoin/pull/27499#pullrequestreview-1535060883)
Concept ACK

👍 hebasto approved a pull request: "subtree: update libsecp256k1 to latest master"
(https://github.com/bitcoin/bitcoin/pull/28093#pullrequestreview-1535192590)
ACK 5080c9c25f44ae9d16a69d41f0da1d1e06483bf7, I've made the `src/secp256k1` subtree update locally and got zero diff with this PR branch.

🤔 furszy reviewed a pull request: "test: Add unit & functional test coverage for blockstore"
(https://github.com/bitcoin/bitcoin/pull/27850#pullrequestreview-1535143803)
Code ACK 2b624780

The introduced unit test document what I see as an error-prone behavior that would be nice to improve in the future:
The `BlockFileInfo` class could be storing information that is not related to the block file it describes.

💬 furszy commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1266840274)
In 2b624780:

nit: could also check the `ReadBlockFromDisk` bool return value.

💬 furszy commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1266861864)
In 2b62478:

nit: could also check that `SaveBlockToDisk` return value is equal to pos2.

Side note:
This behavior smells bad to me. Having `SaveBlockToDisk` updating the block file metadata, without storing the block, and returning successfully looks like an error-prone behavior. The block file info class exist to describe what is inside the block file.

👍 jamesob approved a pull request: "ci: Use DOCKER_BUILDKIT for lint image"
(https://github.com/bitcoin/bitcoin/pull/28083#pullrequestreview-1535304219)
ACK https://github.com/bitcoin/bitcoin/pull/28083/commits/fa2f18ad8e386f067c2d7d5362f6b4dd84c295a0

Built the new linter container locally, verified it works as before. Change definitely simplifies things thanks to BUILDKIT.

Curiously, when I exercised the linter (by adding an unused variable to `test/functional/feature_dbcrash.py`), running the linter with ` docker run --rm -v $(pwd):/bitcoin -w /bitcoin -it bitcoin-linter` didn't actually return a non-zero error code:

```sh
11:18:54 j
...

💬 dergoegge commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1266953778)
We don't do this to any of the other Options we have (afaict). There is also little benefit since this only happens once at startup.

💬 dergoegge commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1266954026)
Done

💬 dergoegge commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1266957788)
Done

💬 dergoegge commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1266954608)
Will leave as is, can be done in a follow up

💬 dergoegge commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1266953921)
Done

💬 dergoegge commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1266941065)
From the developer notes (see discussion in #23810):
```
When casting between integer types, use functional casts such as int(x) or int{x} instead of (int) x.
```

Changed it to a functional cast.

📝 fanquake opened a pull request: "contrib: move user32.dll from bitcoind.exe libs"
(https://github.com/bitcoin/bitcoin/pull/28099)
The user interface library is no-longer needed by `bitcoind.exe`, or utils, only `bitcoin-qt.exe`.
Add missing doc.

🚀 fanquake merged a pull request: "ci: Use DOCKER_BUILDKIT for lint image"
(https://github.com/bitcoin/bitcoin/pull/28083)

💬 furszy commented on pull request "init: return error when block index is non-contiguous, fix feature_init.py file perturbation":
(https://github.com/bitcoin/bitcoin/pull/27823#discussion_r1266974334)
In ad66ca1e:

What if the genesis block index is missing?
The first round of the loop would set `previous_index` to block_index_1. Second one to block_index_2, etc. Making `LoadBlockIndex()` pass.