Bitcoin Core Github
44 subscribers
120K links
Download Telegram
👍 theStack approved a pull request: "test: report detailed msg during utf8 response decoding error"
(https://github.com/bitcoin/bitcoin/pull/31251#pullrequestreview-2427108686)
Tested ACK d65918c5da52c7d5035b4151dee9ffb2e94d4761

Makes sense to provide more detailed information in case of a decoding error.

master:
```
...
responsedata = http_response.read().decode('utf8')
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xc3 in position 42: invalid continuation byte
...
```

PR:
```
...
test_framework.authproxy.JSONRPCException: Cannot decode response in utf8 format, content: b'{"jsonrpc":"2.0","result":{"wallet_name":"\xc3(","backup_path":"/tmp/
...
💬 theStack commented on pull request "test: report detailed msg during utf8 response decoding error":
(https://github.com/bitcoin/bitcoin/pull/31251#discussion_r1836561867)
nit: it might be helpful to still include the original exception message with the decoding error offset and first invalid byte, e.g.
```suggestion
except UnicodeDecodeError as e:
raise JSONRPCException({
'code': -342, 'message': f'Cannot decode response in utf8 format, exception: {e}, content: {data}'})
```
📝 TheCharlatan opened a pull request: "validation: Remove RECENT_CONSENSUS_CHANGE validation result"
(https://github.com/bitcoin/bitcoin/pull/31269)
The *_RECENT_CONSENSUS_CHANGE variants in the validation result enumerations were always unused. They seem to have been kept around speculatively for a soft fork after segwit, however they were never used for taproot either. This points at them not having a clear purpose. Based on the original pull requests' comments their usage was never entirely clear:
https://github.com/bitcoin/bitcoin/pull/11639#issuecomment-370234133 https://github.com/bitcoin/bitcoin/pull/15141#discussion_r271039747

Si
...
💬 fanquake commented on issue "v27.2 guix build fails with hash mismatch":
(https://github.com/bitcoin/bitcoin/issues/31266#issuecomment-2468053269)
> gnutls-3.8.1.tar.xz 2KiB

I'm guessing you got redirected to a bad mirror (or one what has since removed this file etc), and have downloaded a 404 page. The actual tarball should be 6mb. What happens if you retry until you hit a different mirror?

> Strange since there are quite a few v27.2 signatures: https://github.com/bitcoin-core/guix.sigs/tree/main/27.2

Those builders could have had this package built & saved on disk from when 27.0 was released in April.
fanquake closed an issue: "Blockchair"
(https://github.com/bitcoin/bitcoin/issues/31270)
:lock: fanquake locked an issue: "Blockchair"
(https://github.com/bitcoin/bitcoin/issues/31270)
💬 0xB10C commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#issuecomment-2468065957)
> 🐙 This pull request conflicts with the target branch and [needs rebase](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes).

Note that in #26593 the tracepoint macros (e.g. `TRACE7`, `TRACE8`, ..) were renamed to just `TRACEPOINT` (without a number). When rebasing, there will be a few places where these will need to be renamed as you touched code close to it. Similarly, in the last commit ("tracing: pass if replaced by tx/pkg to tracepoint", currently 0ca1e05d8
...
💬 ismaelsadeeq commented on pull request "test: enable running independent functional test sub-tests":
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1836634920)
Done in latest push.
thanks
💬 ismaelsadeeq commented on pull request "test: enable running independent functional test sub-tests":
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1836636121)
leaving this as is; but happy to take explicit suggestions :)
💬 mzumsande commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#discussion_r1836640319)

> > so in current master nothing but 23 or 0 should ever be passed.
>
> So, we could adjust both addrman and connman fuzz targets to get 23 or 0 to use for `max_pct`. Sounds good?

I think fuzzing in range between 0 and 100 would make sense - after all that's what the function is designed to handle
👍 TheCharlatan approved a pull request: "refactor: Drop deprecated space in operator""_mst"
(https://github.com/bitcoin/bitcoin/pull/31267#pullrequestreview-2427254622)
ACK faf21625652fd0d4bbf9b86fd9ebedb5857505ea
👍 l0rinc approved a pull request: "refactor: Drop deprecated space in operator""_mst"
(https://github.com/bitcoin/bitcoin/pull/31267#pullrequestreview-2427272234)
ACK faf21625652fd0d4bbf9b86fd9ebedb5857505ea
💬 l0rinc commented on pull request "refactor: Drop deprecated space in operator""_mst":
(https://github.com/bitcoin/bitcoin/pull/31267#discussion_r1836659165)
nit: given the focus on miniscript here, we might as well use the whole namespace like we do for e.g. `std::literals`:
```suggestion
using namespace miniscript;
```
💬 l0rinc commented on pull request "refactor: Drop deprecated space in operator""_mst":
(https://github.com/bitcoin/bitcoin/pull/31267#discussion_r1836663735)
nit: `Fragment` seems unused
💬 BrandonOdiwuor commented on pull request "doc: Fix gen-manpages to check build options":
(https://github.com/bitcoin/bitcoin/pull/29457#discussion_r1836669430)
@maflcko, are you suggesting the default behaviour should be to also fail if the `enabled components` are missing?
💬 fanquake commented on pull request "refactor: Drop deprecated space in operator""_mst":
(https://github.com/bitcoin/bitcoin/pull/31267#issuecomment-2468185362)
> Clang-20 will do so by default, according to https://clang.llvm.org/docs/DiagnosticsReference.html#wdeprecated-literal-operator.

Checked that main is doing this:
```bash
In file included from /bitcoin/src/test/miniscript_tests.cpp:17:
/bitcoin/src/script/miniscript.h:153:34: warning: identifier '_mst' preceded by whitespace in a literal operator declaration is deprecated [-Wdeprecated-literal-operator]
153 | inline consteval Type operator"" _mst(const char* c, size_t l) {
|
...
💬 hodlinator commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1836690281)
Still rebooting :brain: for this week, sorry for the noise.
💬 maflcko commented on pull request "refactor: Drop deprecated space in operator""_mst":
(https://github.com/bitcoin/bitcoin/pull/31267#discussion_r1836692475)
An explicit namespace can avoid ambiguity and naming collisions, or inconsistencies arising from it, so I'll leave this as-is for now. `std::literals` is different, because there can't be any collisions (as explained in the first link I provided in the pull request description).
💬 maflcko commented on pull request "refactor: Drop deprecated space in operator""_mst":
(https://github.com/bitcoin/bitcoin/pull/31267#discussion_r1836698416)
May do if I retouch, or if clang-tidy is patched to find this. Ref:

```
$ git grep 'using' src/.clang-tidy
src/.clang-tidy:misc-unused-using-decls,