👍 Abdulkbk approved a pull request: "doc: Fix missing comma in JSON example in REST-interface.md"
(https://github.com/bitcoin/bitcoin/pull/31259#pullrequestreview-2427038611)
ACK 5e3b444022c354ad4d69e3142f7bc98d723c9e29
(https://github.com/bitcoin/bitcoin/pull/31259#pullrequestreview-2427038611)
ACK 5e3b444022c354ad4d69e3142f7bc98d723c9e29
🚀 fanquake merged a pull request: "doc: Fix missing comma in JSON example in REST-interface.md"
(https://github.com/bitcoin/bitcoin/pull/31259)
(https://github.com/bitcoin/bitcoin/pull/31259)
💬 willcl-ark commented on pull request "cmake: add optional source files to bitcoin_crypto directly":
(https://github.com/bitcoin/bitcoin/pull/31268#discussion_r1836522285)
I think you have a copy-paste error here. `x86` should be `arm`?
(https://github.com/bitcoin/bitcoin/pull/31268#discussion_r1836522285)
I think you have a copy-paste error here. `x86` should be `arm`?
💬 BrandonOdiwuor commented on pull request "doc: Fix gen-manpages to check build options":
(https://github.com/bitcoin/bitcoin/pull/29457#issuecomment-2467950878)
@laanwj I updated the PR to include the suggestions
(https://github.com/bitcoin/bitcoin/pull/29457#issuecomment-2467950878)
@laanwj I updated the PR to include the suggestions
💬 brunoerg commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#discussion_r1836536074)
> Yes, Assume() is some middle ground between assert() and the softer approach. Assume() + cap in release builds looks reasonable to me too.
I agree. I can address it.
> 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?
(https://github.com/bitcoin/bitcoin/pull/31235#discussion_r1836536074)
> Yes, Assume() is some middle ground between assert() and the softer approach. Assume() + cap in release builds looks reasonable to me too.
I agree. I can address it.
> 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?
💬 maflcko commented on pull request "doc: Fix gen-manpages to check build options":
(https://github.com/bitcoin/bitcoin/pull/29457#discussion_r1836539245)
Not sure about silently ignoring those. The goal of the script is to generate the manpages of the releases, and fail if it can't.
Allowing third parties to generate their own manpages can be implemented behind an option, or not at all.
(https://github.com/bitcoin/bitcoin/pull/29457#discussion_r1836539245)
Not sure about silently ignoring those. The goal of the script is to generate the manpages of the releases, and fail if it can't.
Allowing third parties to generate their own manpages can be implemented behind an option, or not at all.
💬 maflcko commented on pull request "doc: Fix gen-manpages to check build options":
(https://github.com/bitcoin/bitcoin/pull/29457#discussion_r1836536609)
Missing `HAVE_SYSTEM`?
(https://github.com/bitcoin/bitcoin/pull/29457#discussion_r1836536609)
Missing `HAVE_SYSTEM`?
💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-2468000391)
Less overhead for everyone not hooking into the tracepoints :partying_face:. Now that this is merged, here are a few ideas I had for future tracepoint interface work. Noting them for prosperity.
1. We _could_ internalize the relevant macro parts of systemtap's `sys/sdt.h` for the Linux tracepoints. This would allow us to drop the external dependency on systemtap, as we don't use 99% of it. Some prior commentary on this can be found here: https://github.com/hebasto/bitcoin/pull/162#issuecomment
...
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-2468000391)
Less overhead for everyone not hooking into the tracepoints :partying_face:. Now that this is merged, here are a few ideas I had for future tracepoint interface work. Noting them for prosperity.
1. We _could_ internalize the relevant macro parts of systemtap's `sys/sdt.h` for the Linux tracepoints. This would allow us to drop the external dependency on systemtap, as we don't use 99% of it. Some prior commentary on this can be found here: https://github.com/hebasto/bitcoin/pull/162#issuecomment
...
🤔 rkrux reviewed a pull request: "tests: add functional test for miniscript decaying multisig"
(https://github.com/bitcoin/bitcoin/pull/29156#pullrequestreview-2427112510)
@mjdietzx I will review this PR again soon as I want to see a decaying multi-sig using miniscript example checked in. Would you be willing to rebase this over master to get the CMake build changes? The other option is for me (and other reviewers) to use the older build commands to test this PR locally, which is fine as well but at the cost of a much longer build time and having two building tools in my local.
Also, it would be nice to ensure this works with the latest build system though there
...
(https://github.com/bitcoin/bitcoin/pull/29156#pullrequestreview-2427112510)
@mjdietzx I will review this PR again soon as I want to see a decaying multi-sig using miniscript example checked in. Would you be willing to rebase this over master to get the CMake build changes? The other option is for me (and other reviewers) to use the older build commands to test this PR locally, which is fine as well but at the cost of a much longer build time and having two building tools in my local.
Also, it would be nice to ensure this works with the latest build system though there
...
👍 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/
...
(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}'})
```
(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
...
(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
...
⚠️ Asma1665 opened an issue: "Blockchair"
(https://github.com/bitcoin/bitcoin/issues/31270)
https://github.com/Blockchair/ton-indexer/blob/main/Dockerfile
(https://github.com/bitcoin/bitcoin/issues/31270)
https://github.com/Blockchair/ton-indexer/blob/main/Dockerfile
💬 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.
(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)
(https://github.com/bitcoin/bitcoin/issues/31270)
:lock: fanquake locked an issue: "Blockchair"
(https://github.com/bitcoin/bitcoin/issues/31270)
(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
...
(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
(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 :)
(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
(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