👍 pinheadmz approved a pull request: "bumpfee: Allow the user to choose which output is change"
(https://github.com/bitcoin/bitcoin/pull/26467#pullrequestreview-1532950806)
re-ACK e8c31f135c6e9a5f57325dbf4feceafd384f7762

Reviewed all code changes, ran tests locally. I played with the feature in regtest with wallet TXs as well as PSBTs. Made sure invalid input threw errors and expected output was reduced by fee bump. Found one nit below. Maybe remove GUI from OP description? Is that being added in a bitcoin-core/gui PR ?

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK e8c31f135c6e9a5f57325dbf4feceafd38
...

💬 pinheadmz commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1265458854)
nit: extra space?

```suggestion
if (reduce_output.has_value() ? reduce_output.value() == i : OutputIsChange(wallet, output)) {
```

💬 achow101 commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#issuecomment-1638376415)
> > Additionally, a new dialog is added to the GUI that allows the user to select the change output when they are bumping the transaction fee. This dialog will default select output that the wallet determines to be change, if it could find one.
>
> Not seeing this in this branch.

That was moved to https://github.com/bitcoin-core/gui/pull/700. Dropped that from the PR description.

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1265542867)
For context, the issue was originally reported in #17890 and was fixed in #17994. It took me a while to understand exactly what this logic was for and how it worked, so a test to ensure we don't introduce a regression would be helpful!

💬 MarcoFalke commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1638378552)
Looks like it is not set up on Cirrus CI: https://cirrus-ci.com/task/6369879847075840?logs=build#L42 ?

💬 MarcoFalke commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1638380697)
I may try using a full CI VM next.

💬 theStack commented on pull request "refactor: use Span for SipHash::Write":
(https://github.com/bitcoin/bitcoin/pull/28085#discussion_r1265593386)
> Ah, sorry, the `const` prevents a match. You can just drop it.

FWIW, removing the `const` from the Span template type didn't change anything, the error message _"could not match 'Span' against 'vector'"_ still appears. (Or, did I misinterpret your second sentence and with "drop it" you meant to drop the template idea in general?).

@sipa: Fair enough. There are some places already with templates for byte types (e.g. `FastRandomContext::randbytes<B>`, `ParseHex<B>` etc. to name two that I
...

💬 theStack commented on pull request "refactor: use Span for SipHash::Write":
(https://github.com/bitcoin/bitcoin/pull/28085#discussion_r1265593712)
Done.

💬 sipa commented on pull request "refactor: use Span for SipHash::Write":
(https://github.com/bitcoin/bitcoin/pull/28085#discussion_r1265607620)
I wouldn't say it's controversial - I have no problem with adding this.

I just want to question if duplicating lots of things (if here, I suspect there will be several others too) is the right way to go.

📝 fanquake opened a pull request: "subtree: update libsecp256k1 to latest master"
(https://github.com/bitcoin/bitcoin/pull/28093)
Includes https://github.com/bitcoin-core/secp256k1/pull/1378. Which fixes #28079.

💬 fanquake commented on pull request "subtree: update libsecp256k1 to latest master":
(https://github.com/bitcoin/bitcoin/pull/28093#issuecomment-1638514173)
cc @real-or-random @jonasnick

💬 darosior commented on pull request "Descriptors: rule out unspendable miniscript descriptors":
(https://github.com/bitcoin/bitcoin/pull/27997#issuecomment-1638517435)
> No, the rationale was definitely about transaction malleability

Malleability is orthogonal. I was responding to the definition given above of "does the apparent policy of this miniscript match its actual semantics". An `older(42)` script can well be malleated, its apparent policy will match its semantics.

But anyways, let's switch to ##miniscript if we want to continue discussing this? It's not what this PR is implementing anymore.

💬 sipa commented on pull request "refactor: use Span for SipHash::Write":
(https://github.com/bitcoin/bitcoin/pull/28085#issuecomment-1638520565)
utACK 7d92b1430a6fd42c4438810640576830d0ff8d13

💬 fanquake commented on pull request "subtree: update libsecp256k1 to latest master":
(https://github.com/bitcoin/bitcoin/pull/28093#issuecomment-1638522424)
Looks like we'll need to adapt to Windows DLL/symbol export related changes. Will fixup.

🤔 stickies-v reviewed a pull request: "http: add evhttp_connection_set_closecb to avoid g_requests hang"
(https://github.com/bitcoin/bitcoin/pull/27909#pullrequestreview-1533190394)
Nice investigative work, think I found the issue as per my comment.

I wonder if we could track connections instead of requests. A quick implementation (docstrings etc generally not updated, just a PoW) seems to indicate that it does:

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

```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 849e9b482..8fe826f55 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -151,9 +151,9 @@ sta
...

💬 stickies-v commented on pull request "http: add evhttp_connection_set_closecb to avoid g_requests hang":
(https://github.com/bitcoin/bitcoin/pull/27909#discussion_r1265614783)
This is the culprit of [what you're observing](https://github.com/bitcoin/bitcoin/pull/27909#issuecomment-1637992727). `req` becomes a dangling pointer as soon as a request is completed. In most cases (which is why we typically need > 1 number of iterations), this just means that `g_requests.erase(req)` removes 0 elements because it points to a non-existing request, but sometimes (and actually quite frequently as addresses seem to be reused quite aggressively) this will point to an unrelated req
...

💬 real-or-random commented on pull request "subtree: update libsecp256k1 to latest master":
(https://github.com/bitcoin/bitcoin/pull/28093#issuecomment-1638552877)
I believe that this failure is pretty sporadic. So, it may not be worth updating the subtree.

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.

💬 real-or-random commented on pull request "subtree: update libsecp256k1 to latest master":
(https://github.com/bitcoin/bitcoin/pull/28093#issuecomment-1638554188)
Concept ACK (meaning there's nothing wrong with updating the subtree right now)

🤔 stratospher reviewed a pull request: "Make poly1305 support incremental computation + modernize"
(https://github.com/bitcoin/bitcoin/pull/27993#pullrequestreview-1533297514)
tested ACK 4e5c933.

cross checked that this is consistent with poly1305-donna implementation. benchmarks look better compared to previous `poly1305_auth` code too. (also found this [blogpost](https://loup-vaillant.fr/tutorials/poly1305-design) which explains poly1305's design interesting!)

noticed this tiny difference in poly1305-donna's vs [RFC 8439](https://datatracker.ietf.org/doc/html/rfc8439#section-2.5.2) `r` - though it might also be some trick for later computation i guess.
> - r
...

💬 Ayush170-Future commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1265679362)
Thanks, done!