💬 stickies-v commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1266580999)
nit: would not include the ~classname in method name

```suggestion
[[nodiscard]] bool HasCJDNSPrefix() const { return m_addr[0] == 0xfc; }
```

💬 stickies-v commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1266621620)
Maybe let's not move all these lines? A bit unnecessary churn, is fine as is imo.

💬 pinheadmz commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1266640671)
Weird, this comment was posted yesterday but is dated over a month ago (Jun 9). I thought for sure I had tested this case locally and didn't see a missing output -- is this still the case here? Since we check `reduce_output.has_value()` in the if condition, doesn't that mean that if that option is set only the selected option will be designated as change?

💬 real-or-random commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1640091409)
Why are Linux tasks not affected? As far as I understand the blog post, there will be a monthly limit of 40 compute credits (= 40 USD) for all tasks, including Linux tasks. (Or are we already using a persistent worker for Linux?)

💬 tobtoht commented on pull request "depends: xcb-proto 1.15.2":
(https://github.com/bitcoin/bitcoin/pull/28097#issuecomment-1640111808)
Guix builds

```
6d1725c9346bdf04e1eeec2deffda90957bd081700ba896834a143756410fb0e guix-build-7cb88c8b4672/output/aarch64-linux-gnu/SHA256SUMS.part
34486c39daea8a3ce8d92e9c1501f26e5f5e300d9612a25bd56c48ab56353329 guix-build-7cb88c8b4672/output/aarch64-linux-gnu/bitcoin-7cb88c8b4672-aarch64-linux-gnu-debug.tar.gz
cb0080d75d0fc4fc5c7fc022b5771470c60609c35836e7b66c4f479a2edcd098 guix-build-7cb88c8b4672/output/aarch64-linux-gnu/bitcoin-7cb88c8b4672-aarch64-linux-gnu.tar.gz
395c332f8320612581
...

💬 furszy commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1266748837)
> I thought for sure I had tested this case locally and didn't see a missing output -- is this still the case here? Since we check `reduce_output.has_value()` in the if condition, doesn't that mean that if that option is set only the selected option will be designated as change?

Yeah. Seems that I was partially blind yesterday. This issue can still be triggered by a transaction with +2 change outputs and no `reduce_output` (`OutputIsChange()` will be true for them, which overwrites `destChang
...

💬 furszy commented on pull request "wallet: don't duplicate change output if already exist":
(https://github.com/bitcoin/bitcoin/pull/27601#discussion_r1266754466)
> Let's document which part of CoinSelectionParams is modified and which is not.

@S3RK. `CoinSelectionParams` wasn't modified. Only change related code was moved from `CreateTransactionInternal` to `ComputeChangeParams`.

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