💬 achow101 commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1650622058)
ACK 6960c81cbfa6208d4098353e53b313e13a21cb49

🚀 achow101 merged a pull request: "kernel: Remove UniValue from kernel library"
(https://github.com/bitcoin/bitcoin/pull/28113)

📝 ismaelsadeeq opened a pull request: "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain"
(https://github.com/bitcoin/bitcoin/pull/28157)
This PR Follow up comments from [#27622](https://github.com/bitcoin/bitcoin/pull/27622)

It test that the new `regtest-only` option `acceptstalefeeestimates` is not supported on [main, signet and test chains](https://github.com/bitcoin/bitcoin/pull/27622/files#r1235218268), removes an unnecessary [comment](https://github.com/bitcoin/bitcoin/pull/27622/files#r1235204323), and update fee estimator `MAXFILEAGE` [description comment](https://github.com/bitcoin/bitcoin/pull/27622/files#r123388731
...

💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1650639740)
ACK 6960c81cbfa6208d4098353e53b313e13a21cb49

One follow-up could be test coverage for the error case.

@fanquake could you provide more info on how to reproduce your data (e.g. regular build or debug build, the ./configure used, etc.)

I'm not surprised that extraction in the push at d87edf3 didn't make a difference, as the include directives in that version were not optimized for the change. The implementation at https://github.com/jonatack/bitcoin/commits/2023-07-extract-sighashtype-and
...

💬 luke-jr commented on pull request "contrib: Add script to colorize logs":
(https://github.com/bitcoin/bitcoin/pull/26052#issuecomment-1650641281)
Why was this deleted? :/

💬 hebasto commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1650649661)
> If the goal is to stay on a free plan, I think the only option is GitHub Actions CI.

I've tried to port our MSVC build from Cirrus CI to GitHub Actions CI and faced an issue: https://github.com/actions/runner-images/issues/7832. A suggested [workaround](https://github.com/widelands/widelands/pull/6007) did not help. I'm going to investigate it shortly.

💬 achow101 commented on pull request "util: Don't derive secure_allocator from std::allocator":
(https://github.com/bitcoin/bitcoin/pull/27930#issuecomment-1650663419)
ACK 07c59eda00841aafaafd8fd648217b56b1e907c9

🚀 achow101 merged a pull request: "util: Don't derive secure_allocator from std::allocator"
(https://github.com/bitcoin/bitcoin/pull/27930)

💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#issuecomment-1650921567)
After thinking/working through further PRs, I'm beginning to wonder if it isn't better if these classes support incremental computation (i.e., the entire message/packet to be encrypted/decrypted isn't required to be presented in a one (or two) contiguous block(s) of memory, at a single point in time).

The advantage would primarily be a slightly lower latency when receiving a large P2P message. As is, the entire message needs to be received before any decryption can take place. With an increment
...

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