🤔 stickies-v reviewed a pull request: "net, refactor: remove unneeded exports, use helpers over low-level code, use optional"
(https://github.com/bitcoin/bitcoin/pull/28078#pullrequestreview-1534736352)
Approach ACK 5d7188ed92908237ce51bbe6ed0a97472825c6b8

💬 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_r1266591147)
As we're now touching all these LoC, would suggest to clean up even further. Slightly increases the diff but worth it for readability imo.

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

```diff
diff --git a/src/net.cpp b/src/net.cpp
index eee179c8a..b35725ea2 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -154,21 +154,21 @@ uint16_t GetListenPort()
int nBestReachability = -1;
{
LOCK(g_maplocalhost_mutex);
- for (const au
...

💬 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_r1266584113)
nit: is "TODO" easier (i.e. more likely someone does it) to grep than "Note:"? Also, I think these last 2 lines are better placed near the definition of `IsPrivacyNet()`.

💬 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_r1266596572)
Yeah I think that's a good approach.

💬 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,
};
```