💬 MarcoFalke commented on issue "CPU DoS on mainnet in debug mode":
(https://github.com/bitcoin/bitcoin/issues/27586#issuecomment-1554063021)
See also https://github.com/bitcoin/bitcoin/pull/27300#pullrequestreview-1352821536

💬 russeree commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-1554067822)
> Needs rebase on current master, if still relevant

I believe this PR is still very relevant because it substantially improves the logic around address decoding and specifically the displaying of errors in the GUI and RPC. With that said I have rebased and this code is passing all tests other than `Win64 native [vs2022]` which seems to be failing for the majority of PRs that are on master due to warnings.

💬 MarcoFalke commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-1554070439)
Thanks, the reason I mentioned it was the silent merge conflict: `key_io.cpp:152:48: error: ‘const class CChainParams’ has no member named ‘NetworkIDString’`, which is now fixed

💬 russeree commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-1554074157)
> Thanks, the reason I mentioned it was the silent merge conflict: `key_io.cpp:152:48: error: ‘const class CChainParams’ has no member named ‘NetworkIDString’`, which is now fixed

Is there a good way to detect these silent conflicts earlier? Usually I get notified via email when there are issues that need rebased; this is the first time one has happened without a notification.

Thanks

💬 ajtowns commented on pull request "build: debug enable addrman consistency checks":
(https://github.com/bitcoin/bitcoin/pull/27300#discussion_r1198630607)
If these checks are going to be enabled by default on some builds, shouldn't the default be much higher; eg 100 or 1000? Doing it on every call is useful for live debugging or small scale tests, but seems overkill for a node's runtime default?

💬 ajtowns commented on issue "Enable consistency checks by default with `--enable-debug`":
(https://github.com/bitcoin/bitcoin/issues/24709#issuecomment-1554148356)
Not sure this is a good idea? If the consistency check is runtime configurable, why not just configure it at runtime -- anyone who's going to actively debug any problems found isn't going to find it too hard to add a line in bitcoin.conf? If the problem is just that there might be lots of different options and someone might want to get all of them, wouldn't adding an `allchecks=1` option be better? (Or `-checks=addrman -checks=mempool -checks=all` similar to the `-debug=*` option)

The drawbac
...

💬 willcl-ark commented on pull request "build: debug enable addrman consistency checks":
(https://github.com/bitcoin/bitcoin/pull/27300#discussion_r1198635826)
Yes I agree, the performance hit is too much for any kind of normal operation.

I updated the branch with @MarcoFalke's suggestion along with a new debug option (which I called `--enable-debug-checks`, and not `--debug-slow`).

I'll run a few runtime tests on various values for the checks to see if I can find a sweet spot for the frequency, then update the doc commit and push an update here.

💬 MarcoFalke commented on issue "Enable consistency checks by default with `--enable-debug`":
(https://github.com/bitcoin/bitcoin/issues/24709#issuecomment-1554170480)
I think it is pretty clear that we'll have to add at least one more configure flag for (expensive) debug checks. See `--enable-slow-debug` (https://github.com/bitcoin/bitcoin/issues/27586#issuecomment-1553973702) and `--debug-mode` (https://github.com/bitcoin/bitcoin/pull/27300#issuecomment-1479890611). So I fail to see the downside of putting more (runtime configurable) consistency checks into that new flag.

Regardless, it is already questionable whether `--enable-debug` is suitable for prod
...

👍 MarcoFalke approved a pull request: "test: Explicitly specify directory where to search tests for"
(https://github.com/bitcoin/bitcoin/pull/27561#pullrequestreview-1433948180)
lgtm

💬 MarcoFalke commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#discussion_r1198655070)
what is the point of changing this? I'd say to either leave as-is or change to Pathlib, which can use the `/` operator?

💬 MarcoFalke commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1554191949)
Looks like the tests fail in CI, even on a re-run?

👍 dergoegge approved a pull request: "Parallel compact block downloads, take 3"
(https://github.com/bitcoin/bitcoin/pull/27626#pullrequestreview-1433928913)
Code review ACK 42c2696ae58e07de005edf1dda952761f8c9008e

Haven't done a lot of testing yet besides reviewing the tests and running a node with this patch, but the code looks good to me.

(comments are nits, feel free to ignore)

💬 dergoegge commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1198642367)
```suggestion
(pfrom.IsOutboundOrBlockRelayConn() ||
```
I would avoid using CNodeState when possible.

💬 dergoegge commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1198646710)
```suggestion
CNodeState& state = *Assert(State(node_id));
```

💬 dergoegge commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1198645480)
```suggestion
CNodeState& nodestate = *Assert(State(nodeid));
```

⚠️ kallerosenbaum opened an issue: "Sign PSBT: Can't verify change output"
(https://github.com/bitcoin-core/gui/issues/732)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

When I load a PSBT from file to sign it, the dialog that appears looks like this:


There's no way for me to know that the second output belongs to me (it does). Without knowing this I'd feel really uncomfortable signing the PSBT.

### Expected behaviour

The
...

💬 willcl-ark commented on issue "Enable consistency checks by default with `--enable-debug`":
(https://github.com/bitcoin/bitcoin/issues/24709#issuecomment-1554227886)
Before I actually push any more changes, I wonder if the following would work and feel "expected" to developers:

1. `--enable-debug`: behaviour change to disable lockorder/lockcontention consistency checks + behaviour change to disable `boost_multi_index_safe_mode`
2. `--enable-debug-checks`: implies 1. plus enables consistency checks (addrman, lockorder, lockcontention)
3. `--enable-debug-all`?: implies 2. plus enables `boost_multi_index_safe_mode`

The way I had been approaching this wa
...

💬 Sjors commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1554246293)
Rebased because `<key_io.h>` moved (Kernel related?) and to add an explicit `include <univalue.h>` (macOS / clang doesn't care, but CI does and it makes sense anyway).

💬 fanquake commented on issue "ci: failure in Docker build step":
(https://github.com/bitcoin/bitcoin/issues/27492#issuecomment-1554257095)
Closing for now.

✅ fanquake closed an issue: "ci: failure in Docker build step"
(https://github.com/bitcoin/bitcoin/issues/27492)