👍 TheCharlatan approved a pull request: "validation: stricter internal handling of invalid blocks"
(https://github.com/bitcoin/bitcoin/pull/31405#pullrequestreview-2912478922)
Re-ACK f6b782f3aad4a6bcf823a9a0fabb4418bca1eea1
(https://github.com/bitcoin/bitcoin/pull/31405#pullrequestreview-2912478922)
Re-ACK f6b782f3aad4a6bcf823a9a0fabb4418bca1eea1
💬 hebasto commented on pull request "doc: add missing packages for BSDs (cmake, gmake, curl) to depends/README.md":
(https://github.com/bitcoin/bitcoin/pull/32711#discussion_r2137248530)
Strictly speaking, `[cmake-core](https://cgit.freebsd.org/ports/tree/devel/cmake-core)` hould suffice.
(https://github.com/bitcoin/bitcoin/pull/32711#discussion_r2137248530)
Strictly speaking, `[cmake-core](https://cgit.freebsd.org/ports/tree/devel/cmake-core)` hould suffice.
💬 Zk2u commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2958214955)
I don't think people understand that this is _by default_. You can still just set your own limits in your config. If you disagree, you can just change it on your nodes.
This was technically the correct thing to do, even if all the bots on X said otherwise. This whole thing should've been a big nothingburger. Most of you all commenting on the issue aren't understanding the effect of the changes at all. Mob behaviour towards devs should not be tolerated.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2958214955)
I don't think people understand that this is _by default_. You can still just set your own limits in your config. If you disagree, you can just change it on your nodes.
This was technically the correct thing to do, even if all the bots on X said otherwise. This whole thing should've been a big nothingburger. Most of you all commenting on the issue aren't understanding the effect of the changes at all. Mob behaviour towards devs should not be tolerated.
💬 hebasto commented on pull request "doc: add missing packages for BSDs (cmake, gmake, curl) to depends/README.md":
(https://github.com/bitcoin/bitcoin/pull/32711#discussion_r2137304958)
The `perl` package is necessary to provide `shasum`: https://github.com/bitcoin/bitcoin/blob/dff208bd5a14d6c3384a0f55cfefb14c29789c57/depends/builders/netbsd.mk#L1
(https://github.com/bitcoin/bitcoin/pull/32711#discussion_r2137304958)
The `perl` package is necessary to provide `shasum`: https://github.com/bitcoin/bitcoin/blob/dff208bd5a14d6c3384a0f55cfefb14c29789c57/depends/builders/netbsd.mk#L1
💬 lordnakamoto commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2958269968)
> I don't think people understand that this is _by default_. You can still just set your own limits in your config. If you disagree, you can just change it on your nodes.
>
> This was technically the correct thing to do, even if all the bots on X said otherwise. This whole thing should've been a big nothingburger. Most of you all commenting on the issue aren't understanding the effect of the changes at all. Mob behaviour towards devs should not be tolerated.
The real change was allowing mu
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2958269968)
> I don't think people understand that this is _by default_. You can still just set your own limits in your config. If you disagree, you can just change it on your nodes.
>
> This was technically the correct thing to do, even if all the bots on X said otherwise. This whole thing should've been a big nothingburger. Most of you all commenting on the issue aren't understanding the effect of the changes at all. Mob behaviour towards devs should not be tolerated.
The real change was allowing mu
...
💬 maflcko commented on issue "Moving this repo to bitcoin-core":
(https://github.com/bitcoin/bitcoin/issues/32340#issuecomment-2958320638)
I understand the main motivation is philosophical, but there are other practical benefits not yet mentioned. In the context of CI subscriptions ( https://github.com/bitcoin/bitcoin/issues/31965 ), which are usually done on a per-org Github App install, the practical benefit would be to not have to configure/sync everything twice as well (similar to the already mentioned user/team/moderation overhead).
(https://github.com/bitcoin/bitcoin/issues/32340#issuecomment-2958320638)
I understand the main motivation is philosophical, but there are other practical benefits not yet mentioned. In the context of CI subscriptions ( https://github.com/bitcoin/bitcoin/issues/31965 ), which are usually done on a per-org Github App install, the practical benefit would be to not have to configure/sync everything twice as well (similar to the already mentioned user/team/moderation overhead).
💬 waketraindev commented on pull request "init, doc: Replace datacarrier(size) deprecation with non-recommendation text":
(https://github.com/bitcoin/bitcoin/pull/32714#issuecomment-2958341821)
Concept NACK
Using datacarrier for mempool/relay filtering appears to be driven more by ideological or social coercion motivations than technical merit. It's better to just remove it.
As long as there's enough fee attached, these transactions will likely be mined anyway. That means nodes that previously saw and rejected them will end up redownloading them. Also note that vExtraTxnForCompact has a default size of 100 entries, so this kind of filtering is not well-suited for real-world condi
...
(https://github.com/bitcoin/bitcoin/pull/32714#issuecomment-2958341821)
Concept NACK
Using datacarrier for mempool/relay filtering appears to be driven more by ideological or social coercion motivations than technical merit. It's better to just remove it.
As long as there's enough fee attached, these transactions will likely be mined anyway. That means nodes that previously saw and rejected them will end up redownloading them. Also note that vExtraTxnForCompact has a default size of 100 entries, so this kind of filtering is not well-suited for real-world condi
...
💬 hebasto commented on pull request "doc: add missing packages for BSDs (cmake, gmake, curl) to depends/README.md":
(https://github.com/bitcoin/bitcoin/pull/32711#discussion_r2137368061)
On NetBSD 10.1, the default compiler is GCC 10.5.0, which makes necessary installing `gcc14`.
(https://github.com/bitcoin/bitcoin/pull/32711#discussion_r2137368061)
On NetBSD 10.1, the default compiler is GCC 10.5.0, which makes necessary installing `gcc14`.
💬 hodlinator commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2135841016)
nit: Renaming to something like `set_host` would describe better what it does IMHO.
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2135841016)
nit: Renaming to something like `set_host` would describe better what it does IMHO.
🤔 hodlinator reviewed a pull request: "test: enabling wallet migration functional test on windows"
(https://github.com/bitcoin/bitcoin/pull/32219#pullrequestreview-2910271020)
Code review 691e5ba234936020ee47c8f467645bf245ab9fc7
Thanks for incorporating my feedback so far!
Did some testing on Windows. Main concern is about introducing code using legacy python interface.
(https://github.com/bitcoin/bitcoin/pull/32219#pullrequestreview-2910271020)
Code review 691e5ba234936020ee47c8f467645bf245ab9fc7
Thanks for incorporating my feedback so far!
Did some testing on Windows. Main concern is about introducing code using legacy python interface.
💬 hodlinator commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2137082347)
`urllib.request.urlretrieve` is marked as being part of the legacy interface: https://docs.python.org/3/library/urllib.request.html#legacy-interface. Would be good to avoid introducing new code using it.
Here an alternative that is unfortunately a bit more verbose:
```diff
diff --git a/test/get_previous_releases.py b/test/get_previous_releases.py
index 955fcf15d1..7e5c66de0f 100755
--- a/test/get_previous_releases.py
+++ b/test/get_previous_releases.py
@@ -12,6 +12,7 @@ import argparse
...
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2137082347)
`urllib.request.urlretrieve` is marked as being part of the legacy interface: https://docs.python.org/3/library/urllib.request.html#legacy-interface. Would be good to avoid introducing new code using it.
Here an alternative that is unfortunately a bit more verbose:
```diff
diff --git a/test/get_previous_releases.py b/test/get_previous_releases.py
index 955fcf15d1..7e5c66de0f 100755
--- a/test/get_previous_releases.py
+++ b/test/get_previous_releases.py
@@ -12,6 +12,7 @@ import argparse
...
💬 hodlinator commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2137366441)
nit: Could postpone creating the version-directory until we've verified the checksum and are ready to extract. This avoids the block above ("Using cached ...") thinking it has a cached copy in case the code has failed further down and the directory is just empty. (Also slight improvement to error handling).
```diff
diff --git a/test/get_previous_releases.py b/test/get_previous_releases.py
index 955fcf15d1..7e5c66de0f 100755
--- a/test/get_previous_releases.py
+++ b/test/get_previous_relea
...
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2137366441)
nit: Could postpone creating the version-directory until we've verified the checksum and are ready to extract. This avoids the block above ("Using cached ...") thinking it has a cached copy in case the code has failed further down and the directory is just empty. (Also slight improvement to error handling).
```diff
diff --git a/test/get_previous_releases.py b/test/get_previous_releases.py
index 955fcf15d1..7e5c66de0f 100755
--- a/test/get_previous_releases.py
+++ b/test/get_previous_relea
...
💬 hodlinator commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2137276878)
nit: Would suggest sticking to ASCII to avoid this issue on Windows (unless we want a hard requirement to set up the Python env with UTF-8):
```
$ py test/get_previous_releases.py
Releases directory: releases
Fetching: https://bitcoincore.org/bin/bitcoin-core-0.14.3/bitcoin-0.14.3-win64.zip
Downloading: [----------------------------------------] 0.3%
Download failed: 'charmap' codec can't encode characters in position 15-21: character maps to <undefined>
```
```suggestion
bar
...
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2137276878)
nit: Would suggest sticking to ASCII to avoid this issue on Windows (unless we want a hard requirement to set up the Python env with UTF-8):
```
$ py test/get_previous_releases.py
Releases directory: releases
Fetching: https://bitcoincore.org/bin/bitcoin-core-0.14.3/bitcoin-0.14.3-win64.zip
Downloading: [----------------------------------------] 0.3%
Download failed: 'charmap' codec can't encode characters in position 15-21: character maps to <undefined>
```
```suggestion
bar
...
💬 maflcko commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2137397453)
`PYTHONUTF8=1` is required on Windows for running the tests, so it seems fine to require it for the download as well, but no strong opinion. Either char seems fine here.
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2137397453)
`PYTHONUTF8=1` is required on Windows for running the tests, so it seems fine to require it for the download as well, but no strong opinion. Either char seems fine here.
💬 hodlinator commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2137420665)
`$ PYTHONUTF8=1 py test/get_previous_releases.py` worked but I would lean slightly towards ASCII as the error message was so bad (could be mistaken for the downloaded buffer itself somehow being encoded wrongly).
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2137420665)
`$ PYTHONUTF8=1 py test/get_previous_releases.py` worked but I would lean slightly towards ASCII as the error message was so bad (could be mistaken for the downloaded buffer itself somehow being encoded wrongly).
💬 fanquake commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2958443130)
Can you update the PR description to reflect the current state of the PR, it seems like it describes this as one of multiple approaches, and mostly talks about a different approach?
Looking through the changes here, I can't see any documentation about where the `.dat` comes from, who's responsible for generating it (what's required determinism wise), how often it needs updating (who's resposible for that, as well as maintaining any relevant tooling, and where that lives), how any of this effe
...
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2958443130)
Can you update the PR description to reflect the current state of the PR, it seems like it describes this as one of multiple approaches, and mostly talks about a different approach?
Looking through the changes here, I can't see any documentation about where the `.dat` comes from, who's responsible for generating it (what's required determinism wise), how often it needs updating (who's resposible for that, as well as maintaining any relevant tooling, and where that lives), how any of this effe
...
💬 dergoegge commented on pull request "p2p: Add witness mutation check inside FillBlock":
(https://github.com/bitcoin/bitcoin/pull/32646#discussion_r2137430301)
> if the witness merkle check fails due to collision, does that mean that the transaction that collided had the same txid but a different wtxid?
That would be my understanding as well, i.e. the txs are only different in their witness. Pretty sure that is the only scenario, as otherwise the prior merkle root check would fail.
(https://github.com/bitcoin/bitcoin/pull/32646#discussion_r2137430301)
> if the witness merkle check fails due to collision, does that mean that the transaction that collided had the same txid but a different wtxid?
That would be my understanding as well, i.e. the txs are only different in their witness. Pretty sure that is the only scenario, as otherwise the prior merkle root check would fail.
💬 fanquake commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2137463742)
In c78f183800db663bba5f9d3a7280583127eebe3f: It would be good for the commit messages and/or the added doc, to explain that this is (I'm assuming?) only an issue on Windows. If it isn't, and it effects all other platforms, then these test changes could be split out, with a reproducer showing the issue.
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2137463742)
In c78f183800db663bba5f9d3a7280583127eebe3f: It would be good for the commit messages and/or the added doc, to explain that this is (I'm assuming?) only an issue on Windows. If it isn't, and it effects all other platforms, then these test changes could be split out, with a reproducer showing the issue.
💬 hodlinator commented on pull request "init, doc: Replace datacarrier(size) deprecation with non-recommendation text":
(https://github.com/bitcoin/bitcoin/pull/32714#issuecomment-2958563890)
Concept NACK *for v30*
Noting that this PR is opened on the day after 32406 is merged.
Keeping the settings as deprecated for now means the delta to full removal is smaller, while removing the deprecation increases that delta. I think enough boats have been burned already, let's not re-build them only to burn them again. People implying maliciousness behind the first PR will not suddenly have their confidence restored by this one.
What would change my mind:
* New/better arguments for u
...
(https://github.com/bitcoin/bitcoin/pull/32714#issuecomment-2958563890)
Concept NACK *for v30*
Noting that this PR is opened on the day after 32406 is merged.
Keeping the settings as deprecated for now means the delta to full removal is smaller, while removing the deprecation increases that delta. I think enough boats have been burned already, let's not re-build them only to burn them again. People implying maliciousness behind the first PR will not suddenly have their confidence restored by this one.
What would change my mind:
* New/better arguments for u
...
💬 fanquake commented on pull request "rpc, doc: update `listdescriptors` RCP help":
(https://github.com/bitcoin/bitcoin/pull/32708#issuecomment-2958584077)
Partial backport to `29.x` in #32589.
(https://github.com/bitcoin/bitcoin/pull/32708#issuecomment-2958584077)
Partial backport to `29.x` in #32589.