💬 MarcoFalke commented on pull request "test: fix `feature_addrman.py` on big-endian systems":
(https://github.com/bitcoin/bitcoin/pull/27529#issuecomment-1651013529)
tested as well, so my recommendation would be to merge this before #https://github.com/bitcoin/bitcoin/pull/28087

💬 MarcoFalke commented on pull request "refactor: consistently use ApplyArgsManOptions for PeerManager::Options":
(https://github.com/bitcoin/bitcoin/pull/28148#discussion_r1274407349)
unrelated in 8a3159728ae84cb8093e2e9fa5d2c2b0a7d545da: I think peermanager options have nothing to do with addrmanager options, so this could be moved down, closer to where it is used.

💬 MarcoFalke commented on pull request "refactor: consistently use ApplyArgsManOptions for PeerManager::Options":
(https://github.com/bitcoin/bitcoin/pull/28148#issuecomment-1651027508)
lgtm ACK 8a3159728ae84cb8093e2e9fa5d2c2b0a7d545da

💬 MarcoFalke commented on pull request "fuzz: add target for `ScriptPubKeyMan` (legacy)":
(https://github.com/bitcoin/bitcoin/pull/28153#discussion_r1274419571)
fuzz targets should be deterministic based on the fuzz input. If you use a global wallet and spkm, the global state from the previous fuzz inputs will leak into the current one.

⚠️ russeree opened an issue: "Add Bech32.cpp LocateErrors case to check for minimum length."
(https://github.com/bitcoin/bitcoin/issues/28159)
### Please describe the feature you'd like to see added.

Add a "Bech32 string too short" case to Bech32.cpp LocateErrors to reduce ambiguity and collisions with "Invalid separator position" case.

### Is your feature related to a problem, if so please describe it.

Error representation when calling LocateErrors within ```src/test/bech32_tests.cpp``` is ambiguous and incorrect in some cases because of a missing bech32 string to short case. Not having a too short case causes a fall though into th
...

💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1274514091)
Fixed in #28157

💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1274514642)
Fixed in #28157

💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1274515246)
Fixed in #28157

📝 russeree opened a pull request: "Bech32 LocateErrors "Bech32 string too short" case"
(https://github.com/bitcoin/bitcoin/pull/28160)
As a reference issue https://github.com/bitcoin/bitcoin/issues/28159 has additional details and rationale. This PR adds an additional if case to bech32.cpp LocateErrors to account for the Bech32 string too short case. This clears up ambiguity where a Bech32 string that doesn't meet the specs of BIP173 generates a separator position or missing error. This was discovered while checking and running the src/test/bech32_tests.cpp file.

Adding this case would make DecodeDestination more accurate o
...

💬 fanquake commented on issue "Add Bech32.cpp LocateErrors case to check for minimum length.":
(https://github.com/bitcoin/bitcoin/issues/28159#issuecomment-1651203036)
I don't think there's a need to open an issue if you're going to open a PR 10 minutes later. Would suggest combining anything relevant from here into the PR description, and save splitting up the discussion.

✅ fanquake closed an issue: "Add Bech32.cpp LocateErrors case to check for minimum length."
(https://github.com/bitcoin/bitcoin/issues/28159)

💬 russeree commented on issue "Add Bech32.cpp LocateErrors case to check for minimum length.":
(https://github.com/bitcoin/bitcoin/issues/28159#issuecomment-1651205286)
I apologize for the double posting, I haven't seen others do it in the past. Lesson learned.

💬 MarcoFalke commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1274568557)
Not sure about adding dead code. Before on master this will already throw and never reach this part of the code:

```
test_framework.authproxy.JSONRPCException: Wrong type passed:
{
"Position 3 (sighashtype)": "JSON value of type bool is not of expected type string"
} (-3)
```

💬 MarcoFalke commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1274567555)
Is this true? Before on master this will already throw:

```
test_framework.authproxy.JSONRPCException: Wrong type passed:
{
"Position 3 (sighashtype)": "JSON value of type bool is not of expected type string"
} (-3)
```

🚀 fanquake merged a pull request: "test: Ignore UTF-8 errors in assert_debug_log"
(https://github.com/bitcoin/bitcoin/pull/28035)

💬 MarcoFalke commented on pull request "util: Remove DirIsWritable, GetUniquePath":
(https://github.com/bitcoin/bitcoin/pull/28075#discussion_r1274571873)
cc @TheCharlatan (libbitcoinkernel)

🚀 fanquake merged a pull request: "valgrind: add suppression for bug 472219"
(https://github.com/bitcoin/bitcoin/pull/28145)

🚀 fanquake merged a pull request: "test: fix `feature_addrman.py` on big-endian systems"
(https://github.com/bitcoin/bitcoin/pull/27529)

🚀 fanquake merged a pull request: "suppressions: note that `type:ClassName::MethodName` should be used"
(https://github.com/bitcoin/bitcoin/pull/28147)

💬 carnhofdaki commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#issuecomment-1651257804)
@sipa I think that the disadvantages you mention are much worse than the advantages it would bring. I think that keeping it simple is (at least in this implementation intended for first appearance in the `master` branch) an important feature of BIP-324.

As for me and my poor old Bitcoin node,
please do not add complexity to the code.