💬 sipa commented on pull request "refactor: use Span for SipHash::Write":
(https://github.com/bitcoin/bitcoin/pull/28085#discussion_r1265489980)
While I don't obect to having this particular function templatized, I don't think it's a scalable approach to introduce dual char/byte functions all over the codebase, just because not everything is using bytes.

IMO, the goal should be to *only* have a byte interface here; The options for that are:
* Just ~byte~ bite the bullet now, and introduce `MakeByteSpan` in all call sites.
* Stick with a `Span<const unsigned char>` interface for now, and revisit at some point in the future when more
...

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1265513530)
The idea here is that this check is part of a heuristic that we use when we receive a block that was not requested. For anti-DoS reasons, back in #5875 we introduced logic to generally not process unrequested blocks, but we left in an optimization that was intended to not throw away blocks that would immediately advance our tip, in case we had some peer (like Matt's old fast-relay-network, if I remember right) that was sending such blocks (see eg comment here https://github.com/bitcoin/bitcoin/
...

💬 MarcoFalke commented on pull request "fuzz: use `ConnmanTestMsg` in `connman`":
(https://github.com/bitcoin/bitcoin/pull/28091#issuecomment-1638355685)
lgtm ACK d9bd559638b453b8c6e03e34005ad5c52099b6bf

💬 MarcoFalke commented on pull request "test: fix `feature_addrman.py` on big-endian systems":
(https://github.com/bitcoin/bitcoin/pull/27529#issuecomment-1638360511)
See https://github.com/bitcoin/bitcoin/pull/28087

💬 fanquake commented on pull request "build: document that -Wreturn-type has been fixed upstream (mingw-w64)":
(https://github.com/bitcoin/bitcoin/pull/28092#issuecomment-1638363452)
Remembered why we can't make this change; because the current code was added to our buildsystem as a hack to fix the Windows CI under `-Werror`, rather than just suppressing the incorrect warning... Took my own advice from 25972 and deleted the check entirely, adding `-Wno-return-type` to the CI, where it can be dropped once we are using a fixed version of the headers.

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1638374417)
Just pushed a new branch incorporating feedback from all of @ryanofsky's prior review. Regarding https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1251289117, I decided that it made the most sense to invoke `ActivateBestChain()` on both chainstates in `LoadExternalBlockFile` if pruning was enabled, even though I think it'll take some more work to figure out how pruning should function in the event that we're in the middle of assumeutxo-initiated background validation.

Regarding the c
...

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