💬 glozow commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1266760855)
Ah good point, it's not necessary, we could just use `GenTxid::Wtxid` for all of them. :+1:

💬 hebasto commented on pull request "depends: xcb-proto 1.15.2":
(https://github.com/bitcoin/bitcoin/pull/28097#issuecomment-1640224241)
Concept ACK.

💬 fanquake commented on pull request "ci: document that -Wreturn-type has been fixed upstream (mingw-w64)":
(https://github.com/bitcoin/bitcoin/pull/28092#issuecomment-1640248764)
Ok. Done with less embedding. Only difference between this and master is `s/-Wno-error=return-type/-Wno-return-type/`.

i.e master https://cirrus-ci.com/task/6747293471211520?logs=ci#L1367 vs this PR https://cirrus-ci.com/task/5094810491551744?logs=ci#L983.

💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1266817621)
ah awesome thank you, I'll go with `204 No Response`

💬 jonasnick commented on pull request "subtree: update libsecp256k1 to latest master":
(https://github.com/bitcoin/bitcoin/pull/28093#issuecomment-1640300067)
> There's also still (sorry!) the sporadic test condition failed: ((~x[m][shift]) << (64 - (1 << usebits))) == 0 failure (https://github.com/bitcoin-core/secp256k1/pull/367, https://github.com/bitcoin-core/secp256k1/issues/610). That one will be solved by https://github.com/bitcoin-core/secp256k1/pull/1298 by @sipa. Perhaps we should prioritize this PR and wait for its merge first.

This is fixed in libsecp256k1 master now.

🤔 furszy reviewed a pull request: "bumpfee: Allow the user to choose which output is change"
(https://github.com/bitcoin/bitcoin/pull/26467#pullrequestreview-1535090127)
Code review ACK e8c31f13

💬 furszy commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1266806776)
In 7d83502d:

The first sentence `"The 0-based index of the output from which the additional fees will be deducted"` isn't totally accurate to what will happen.

If the `bumpfee` process adds another input to satisfy the new fee, the `reduce_output` output will also be increased by the new `inputs - outputs` surplus.

💬 fanquake commented on pull request "subtree: update libsecp256k1 to latest master":
(https://github.com/bitcoin/bitcoin/pull/28093#issuecomment-1640336092)
> This is fixed in libsecp256k1 master now.

Thanks. Updated to pull that change in here as well.

💬 stickies-v commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1266842776)
nit: I find designated initializers a bit more readable but maybe that's personal? (here and in `setup_common.cpp`)

```suggestion
PeerManager::Options peerman_opts{
.banman = node.banman.get(),
.ignore_incoming_txs = ignores_incoming_txs,
};
```

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