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

🚀 fanquake merged a pull request: "test: Avoid intermittent issues due to async events in validationinterface_tests"
(https://github.com/bitcoin/bitcoin/pull/28150)

📝 MarcoFalke opened a pull request: "ci: Move ASan USDT to persistent_worker"
(https://github.com/bitcoin/bitcoin/pull/28161)
To run the USDT functional tests, the ASan task currently requires the container host to run the Ubuntu Lunar Linux kernel (or later). Cirrus CI is the only provider that allows to spin up full VMs, however they will start to charge for all tasks (See slightly related discussion in https://github.com/bitcoin/bitcoin/issues/28098).

Since it is cheaper and recommended by Cirrus CI to just run a persistent worker, do that.