💬 pinheadmz commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#issuecomment-1638272123)
> 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.

💬 MarcoFalke commented on pull request "refactor: use Span for SipHash::Write":
(https://github.com/bitcoin/bitcoin/pull/28085#discussion_r1265463653)
Ah, sorry, the `const` prevents a match. You can just drop it. Or alternatively put in the header two overloads of the form:

```cpp
CSipHasher& Write(Span<const unsigned char> data);
CSipHasher& Write(Span<const std::byte> data);
```

And the make one call the other in the cpp file.

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1265477633)
We may not have received the snapshot base block, and it's only when we call `ReceivedBlockTransactions` on a block that we set VALID_TRANSACTIONS on the block index entry. So the snapshot base block may not otherwise be considered a block index candidate, but the snapshot base block could be the tip of the snapshot chain so we need to be able to call `TryAddBlockIndexCandidate()`.

I'll push an update that adds a comment explaining this.

💬 furszy commented on pull request "test: Bump walletpassphrase timeout to avoid intermittent issue":
(https://github.com/bitcoin/bitcoin/pull/28089#discussion_r1265478128)
> An alternative might be to not use a passphrase after this test

Yeah. Would be even better to create a wallet specific to this test and unload it at the end of it.

> Do you want to submit either as a pull request? Happy to close mine then. Otherwise, I'll leave the feedback for a follow-up, as the goal here is to not change any logic, only the constant.

Will try to do it later today. Shouldn't take me much.
Still, all good if this gets merged before it.

💬 Crypt-iQ commented on pull request "net: disconnect inside AttemptToEvictConnection":
(https://github.com/bitcoin/bitcoin/pull/27912#issuecomment-1638306663)
> OK I've pushed a new set of changes which now disconnects nodes synchronously inside of `AttemptToEvictConnection`.
>
> @Crypt-iQ I'd be curious if you still see these new changes as resolving the issue in #27843? I havent' gotten your test patch working to my satisfaction yet (or at least, I don't see positive eviction candidate selection during it so it wouldn't overflow `nMaxInbound` even without these changes).

Sorry I have been a bit short on time recently, but let me know when to t
...

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