💬 KaueTech commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3160926752)
> I can sympapthezie somewhat with wanting an un-polluted system as a developer, but for that I personally think Nix is a strictly superior solution to docker for this use-case (and indeed I maintain a nix devShell flake [here](https://github.com/bitcoin-dev-tools/bix) for exactly this purpose).
>
> We already have our CI setup containerised for exactly this type of reason too, and the use-case of a developer wanting to build-and-test inside a container is already provided by running a CI job
...
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3160926752)
> I can sympapthezie somewhat with wanting an un-polluted system as a developer, but for that I personally think Nix is a strictly superior solution to docker for this use-case (and indeed I maintain a nix devShell flake [here](https://github.com/bitcoin-dev-tools/bix) for exactly this purpose).
>
> We already have our CI setup containerised for exactly this type of reason too, and the use-case of a developer wanting to build-and-test inside a container is already provided by running a CI job
...
💬 sipa commented on pull request "net, validation: don't punish peers for consensus-invalid txs":
(https://github.com/bitcoin/bitcoin/pull/33050#issuecomment-3160927878)
Concept ACK.
I agree that the current ability to distinguish consensus vs standardness failures doesn't really achieve much. It is not the case that consensus-invalidity is inherently more harmful to us (in terms of DoS potential) than non-standardness violations, and attackers can today already mask any consensus-invalidity by causing a non-standardness to be detected first, evading the punishment.
So it seems the benefit of this distinction is largely so we can kick peers with diverging
...
(https://github.com/bitcoin/bitcoin/pull/33050#issuecomment-3160927878)
Concept ACK.
I agree that the current ability to distinguish consensus vs standardness failures doesn't really achieve much. It is not the case that consensus-invalidity is inherently more harmful to us (in terms of DoS potential) than non-standardness violations, and attackers can today already mask any consensus-invalidity by causing a non-standardness to be detected first, evading the punishment.
So it seems the benefit of this distinction is largely so we can kick peers with diverging
...
💬 achow101 commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3160932660)
> Can we merge this, the builds seem fine
No, there are no ACKs
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3160932660)
> Can we merge this, the builds seem fine
No, there are no ACKs
💬 mzumsande commented on pull request "test: fix p2p_leak_tx.py":
(https://github.com/bitcoin/bitcoin/pull/33121#issuecomment-3160965249)
> I tried some modifications of the test locally but wasn't able to enter the "else branch" where "tx was already announced to us". Were you able to test it?
No, but with a `time.sleep(3)` before `inbound_peer.send_and_ping(want_tx)` it would go into that branch regularly. because the `NextInvToInbounds` will often generate a random interval `<3s` - that's why I mentioned it above.
In general I was a bit unsure about the main point of the subtest (maybe @maflcko you can help?). Is the goal
...
(https://github.com/bitcoin/bitcoin/pull/33121#issuecomment-3160965249)
> I tried some modifications of the test locally but wasn't able to enter the "else branch" where "tx was already announced to us". Were you able to test it?
No, but with a `time.sleep(3)` before `inbound_peer.send_and_ping(want_tx)` it would go into that branch regularly. because the `NextInvToInbounds` will often generate a random interval `<3s` - that's why I mentioned it above.
In general I was a bit unsure about the main point of the subtest (maybe @maflcko you can help?). Is the goal
...
💬 janb84 commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3160965542)
> > Concept NACK
> > What is the usecase for this ? forks would also fork the GUIX build script and if this is intended as multi client build script than move it to an independent repo.
>
> This makes conducting a fork easier, and rebasing the fork/absorbing cores changes if required in a fork easier for a maintainer.
>
> The code is also a lot cleaner, given we check for bitcoin-utils in a better way,
>
> and don't have an unnecessary Bitcoin-Core based text in the build.sh
Thanks
...
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3160965542)
> > Concept NACK
> > What is the usecase for this ? forks would also fork the GUIX build script and if this is intended as multi client build script than move it to an independent repo.
>
> This makes conducting a fork easier, and rebasing the fork/absorbing cores changes if required in a fork easier for a maintainer.
>
> The code is also a lot cleaner, given we check for bitcoin-utils in a better way,
>
> and don't have an unnecessary Bitcoin-Core based text in the build.sh
Thanks
...
💬 achow101 commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3160971980)
NACK
If this is for users to be able to deploy Bitcoin Core in docker, I don't think it belongs in this repo. We have the separate packaging repo where stuff related to producing packages for distribution on specific platforms, so a dockerfile for users to use belongs there.
If this is for development, it does not seem to be useful. It hardcodes a bunch of stuff, including disabling benchmarks and tests. There doesn't seem to be any way to override those options without modifying the docke
...
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3160971980)
NACK
If this is for users to be able to deploy Bitcoin Core in docker, I don't think it belongs in this repo. We have the separate packaging repo where stuff related to producing packages for distribution on specific platforms, so a dockerfile for users to use belongs there.
If this is for development, it does not seem to be useful. It hardcodes a bunch of stuff, including disabling benchmarks and tests. There doesn't seem to be any way to override those options without modifying the docke
...
💬 KaueTech commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3161008257)
> NACK
>
> Se isso for para que os usuários possam implantar o Bitcoin Core no Docker, acho que não pertence a este repositório. Temos um repositório de empacotamento separado, onde estão as coisas relacionadas à produção de pacotes para distribuição em plataformas específicas, então um dockerfile para os usuários usarem deve estar lá.
>
> Se for para desenvolvimento, não parece ser útil. Ele codifica um monte de coisas, incluindo a desativação de benchmarks e testes. Não parece haver nenh
...
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3161008257)
> NACK
>
> Se isso for para que os usuários possam implantar o Bitcoin Core no Docker, acho que não pertence a este repositório. Temos um repositório de empacotamento separado, onde estão as coisas relacionadas à produção de pacotes para distribuição em plataformas específicas, então um dockerfile para os usuários usarem deve estar lá.
>
> Se for para desenvolvimento, não parece ser útil. Ele codifica um monte de coisas, incluindo a desativação de benchmarks e testes. Não parece haver nenh
...
🤔 Aris-Ritz reviewed a pull request: "Add Docker support with multi-arch build and user permissions handling"
(https://github.com/bitcoin/bitcoin/pull/33139#pullrequestreview-3093638317)
.dockerignore
(https://github.com/bitcoin/bitcoin/pull/33139#pullrequestreview-3093638317)
.dockerignore
💬 KaueTech commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3161064917)
You already have your own CI setup and build methods, but they're like using a cannon to kill a fly. I believe the official repository should include a simple way to run Bitcoin Core without worrying about the operating system, dependencies, or anything like that — everything could be handled through a Dockerfile.
As I mentioned, people already use forks of Bitcoin Core like Umbrel’s or Start9’s, which include Dockerfiles. People install Bitcoin Core like any other app, and it just magically
...
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3161064917)
You already have your own CI setup and build methods, but they're like using a cannon to kill a fly. I believe the official repository should include a simple way to run Bitcoin Core without worrying about the operating system, dependencies, or anything like that — everything could be handled through a Dockerfile.
As I mentioned, people already use forks of Bitcoin Core like Umbrel’s or Start9’s, which include Dockerfiles. People install Bitcoin Core like any other app, and it just magically
...
💬 sipa commented on pull request "validation: detect witness stripping without re-running Script checks":
(https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3161082300)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3161082300)
Concept ACK
🤔 murchandamus reviewed a pull request: "refactor: CFeeRate encapsulates FeeFrac internally"
(https://github.com/bitcoin/bitcoin/pull/32750#pullrequestreview-3093620724)
code review, lightly tested ACK d3b8a54a81209420ef6447dd4581e1b6b8550647
> * Passing negative virtual bytes to `GetFee` will return -1 as the fee.
From experimenting a bit, it seems to me that before this PR, a negative vsize would wrap from max value, i.e. `GetFee(<negative>)` would return huge amounts (here with a feerate of 1000 s/kvB):
```
./test/amount_tests.cpp(40): error: in "amount_tests/GetFeeTest": check feeRate.GetFee(-1) == CAmount(-1) has failed [4294967295 != -1]
./test/
...
(https://github.com/bitcoin/bitcoin/pull/32750#pullrequestreview-3093620724)
code review, lightly tested ACK d3b8a54a81209420ef6447dd4581e1b6b8550647
> * Passing negative virtual bytes to `GetFee` will return -1 as the fee.
From experimenting a bit, it seems to me that before this PR, a negative vsize would wrap from max value, i.e. `GetFee(<negative>)` would return huge amounts (here with a feerate of 1000 s/kvB):
```
./test/amount_tests.cpp(40): error: in "amount_tests/GetFeeTest": check feeRate.GetFee(-1) == CAmount(-1) has failed [4294967295 != -1]
./test/
...
💬 murchandamus commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2257860390)
I was curious whether this test would have passed prior to this change, because the vsize was then expected to be an unsigned integer, and it does.
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2257860390)
I was curious whether this test would have passed prior to this change, because the vsize was then expected to be an unsigned integer, and it does.
💬 l0rinc commented on pull request "build: Set AUTHOR_WARNING on warnings":
(https://github.com/bitcoin/bitcoin/pull/33144#issuecomment-3161213864)
Rebased, tested it with:
```patch
diff --git a/CMakeLists.txt b/CMakeLists.txt
--- a/CMakeLists.txt (revision 27e4b8c6856194fa8db028b6f7356f83ea3d7e3a)
+++ b/CMakeLists.txt (date 1754504896922)
@@ -186,6 +186,7 @@
string(APPEND CMAKE_CXX_LINK_EXECUTABLE " ${APPEND_LDFLAGS}")
set(configure_warnings)
+list(APPEND configure_warnings "Testing https://github.com/bitcoin/bitcoin/pull/33144")
include(CheckLinkerSupportsPIE)
check_linker_supports_pie(configure_warnings)
```
running
...
(https://github.com/bitcoin/bitcoin/pull/33144#issuecomment-3161213864)
Rebased, tested it with:
```patch
diff --git a/CMakeLists.txt b/CMakeLists.txt
--- a/CMakeLists.txt (revision 27e4b8c6856194fa8db028b6f7356f83ea3d7e3a)
+++ b/CMakeLists.txt (date 1754504896922)
@@ -186,6 +186,7 @@
string(APPEND CMAKE_CXX_LINK_EXECUTABLE " ${APPEND_LDFLAGS}")
set(configure_warnings)
+list(APPEND configure_warnings "Testing https://github.com/bitcoin/bitcoin/pull/33144")
include(CheckLinkerSupportsPIE)
check_linker_supports_pie(configure_warnings)
```
running
...
💬 optout21 commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33094#issuecomment-3161302880)
> Note that `== 1`/`!= 1`/`< 1` only apply to simple [maps](https://en.cppreference.com/w/cpp/container/map/contains)/[sets](https://en.cppreference.com/w/cpp/container/set/contains) and had to be changed manually.
I understand the `== 1`/`!= 1` parts, but for `< 1`: it means that the value can be only 0, which means "doesn't contain", so it applies equally to de-duplicated containers (map, set) and duplicated containers as well, isn't it?
(https://github.com/bitcoin/bitcoin/pull/33094#issuecomment-3161302880)
> Note that `== 1`/`!= 1`/`< 1` only apply to simple [maps](https://en.cppreference.com/w/cpp/container/map/contains)/[sets](https://en.cppreference.com/w/cpp/container/set/contains) and had to be changed manually.
I understand the `== 1`/`!= 1` parts, but for `< 1`: it means that the value can be only 0, which means "doesn't contain", so it applies equally to de-duplicated containers (map, set) and duplicated containers as well, isn't it?
💬 l0rinc commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33094#issuecomment-3161354674)
Yes, applies to any kind of map, but I had to find and change those manually - clang-tidy doesn't detect or fix those.
(https://github.com/bitcoin/bitcoin/pull/33094#issuecomment-3161354674)
Yes, applies to any kind of map, but I had to find and change those manually - clang-tidy doesn't detect or fix those.
💬 optout21 commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33094#issuecomment-3161358887)
ACK 05153600af10c322e40d107b8cf0c8e69b8b0aeb
- The changes increase code readability, as 'contains' expresses the code logic / intent more specifically
- It also results in higher performance, due to potential early exit. The improvement is probably negligible though.
- Changes are localized (each to a single line), local impact only
(I've reviewed the proposed changed, but didn't look for other potential changes)
(https://github.com/bitcoin/bitcoin/pull/33094#issuecomment-3161358887)
ACK 05153600af10c322e40d107b8cf0c8e69b8b0aeb
- The changes increase code readability, as 'contains' expresses the code logic / intent more specifically
- It also results in higher performance, due to potential early exit. The improvement is probably negligible though.
- Changes are localized (each to a single line), local impact only
(I've reviewed the proposed changed, but didn't look for other potential changes)
🤔 optout21 reviewed a pull request: "refactor: unify container presence checks"
(https://github.com/bitcoin/bitcoin/pull/33094#pullrequestreview-3093962887)
ACK 05153600af10c322e40d107b8cf0c8e69b8b0aeb
- The changes increase code readability, as 'contains' expresses the code logic / intent more specifically
- It also results in higher performance, due to potential early exit. The improvement is probably negligible though.
- Changes are localized (each to a single line), local impact only
(I've reviewed the proposed changed, but didn't look for other potential changes)
(https://github.com/bitcoin/bitcoin/pull/33094#pullrequestreview-3093962887)
ACK 05153600af10c322e40d107b8cf0c8e69b8b0aeb
- The changes increase code readability, as 'contains' expresses the code logic / intent more specifically
- It also results in higher performance, due to potential early exit. The improvement is probably negligible though.
- Changes are localized (each to a single line), local impact only
(I've reviewed the proposed changed, but didn't look for other potential changes)
🤔 glozow reviewed a pull request: "wallet, rpc: add v3 transaction creation and wallet support"
(https://github.com/bitcoin/bitcoin/pull/32896#pullrequestreview-3093839590)
ACK 43a479ca688, only minor nits
(https://github.com/bitcoin/bitcoin/pull/32896#pullrequestreview-3093839590)
ACK 43a479ca688, only minor nits
💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2258108951)
Could make this version 2 to be explicit
Also, could make this more efficient by reusing the outputs vector
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2258108951)
Could make this version 2 to be explicit
Also, could make this more efficient by reusing the outputs vector
💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2258016408)
Is this supposed to say "we are spending an unconfirmed TRUC transaction" ? Also, can be one line?
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2258016408)
Is this supposed to say "we are spending an unconfirmed TRUC transaction" ? Also, can be one line?