💬 l0rinc commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3162052400)
I'm not sure why it would be our job to make the code more forkable - but we do need to make it maintainable, so concept ACK for less hard-coding - no opinion on the implementation details, will let others review the specifics.
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3162052400)
I'm not sure why it would be our job to make the code more forkable - but we do need to make it maintainable, so concept ACK for less hard-coding - no opinion on the implementation details, will let others review the specifics.
📝 bethoffman opened a pull request: "doc:Fix typos"
(https://github.com/bitcoin/bitcoin/pull/33148)
Minor PR to fix spelling
(https://github.com/bitcoin/bitcoin/pull/33148)
Minor PR to fix spelling
💬 l0rinc commented on pull request "doc:Fix typos":
(https://github.com/bitcoin/bitcoin/pull/33148#discussion_r2258736822)
This was already proposed upstream at https://github.com/google/leveldb/pull/769 and https://github.com/google/leveldb/pull/739/files - but since that repo is basically dead, you can propose it to our fork in https://github.com/bitcoin-core/leveldb-subtree instead (where it will also likely be ignored since there's an opportunity cost for spending time with these)
(https://github.com/bitcoin/bitcoin/pull/33148#discussion_r2258736822)
This was already proposed upstream at https://github.com/google/leveldb/pull/769 and https://github.com/google/leveldb/pull/739/files - but since that repo is basically dead, you can propose it to our fork in https://github.com/bitcoin-core/leveldb-subtree instead (where it will also likely be ignored since there's an opportunity cost for spending time with these)
🤔 octavio12345300 reviewed a pull request: "doc:Fix typos"
(https://github.com/bitcoin/bitcoin/pull/33148#pullrequestreview-3094980543)
LIST
(https://github.com/bitcoin/bitcoin/pull/33148#pullrequestreview-3094980543)
LIST
💬 l0rinc commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#issuecomment-3162361510)
I ran the 3 versions you mentioned with hyperfine, 10 runs each:
```python
COMMITS="d767503b6a2618e0c99407acf98f3bd19fb7defd 5bb380ec87af93ed5543ba10e1f8465aca7bcfe7 35e91b899dac689b24a8a95fcef083affc695d1e"; \
PARALLEL=200; RUNS=10; \
CC=gcc; CXX=g++; \
...
(https://github.com/bitcoin/bitcoin/pull/33141#issuecomment-3162361510)
I ran the 3 versions you mentioned with hyperfine, 10 runs each:
```python
COMMITS="d767503b6a2618e0c99407acf98f3bd19fb7defd 5bb380ec87af93ed5543ba10e1f8465aca7bcfe7 35e91b899dac689b24a8a95fcef083affc695d1e"; \
PARALLEL=200; RUNS=10; \
CC=gcc; CXX=g++; \
...
💬 sipa commented on pull request "cmake: Switch to generated `ts_files.cmake` file":
(https://github.com/bitcoin/bitcoin/pull/33115#issuecomment-3162395975)
The changes in `src/qt/bitcoin_szl.ts` look like they contain Rust source code?
(https://github.com/bitcoin/bitcoin/pull/33115#issuecomment-3162395975)
The changes in `src/qt/bitcoin_szl.ts` look like they contain Rust source code?
💬 Ataraxia009 commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3162400137)
> You are free to fork the code and change whatever you like, but the burden of maintaining your fork falls on you not the contributors of Bitcoin Core.
good call, there was a problem with using binary.name in the new python lief version. We should be fine now, just pushed a fix.
However i cant run the drahbot build, if somebody could add the label for building, would be helpful
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3162400137)
> You are free to fork the code and change whatever you like, but the burden of maintaining your fork falls on you not the contributors of Bitcoin Core.
good call, there was a problem with using binary.name in the new python lief version. We should be fine now, just pushed a fix.
However i cant run the drahbot build, if somebody could add the label for building, would be helpful
💬 Ataraxia009 commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3162407197)
> You are free to fork the code and change whatever you like, but the burden of maintaining your fork falls on you not the contributors of Bitcoin Core.
The contributors and builders of the bitcoin network do have a responsibility towards helping decentralise the network, which leads to overall network health.
In my humble opinion, helping support more diversity in client choice for node runners should be, if not already is, a core principle of the network.
Also this leads to less hardc
...
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3162407197)
> You are free to fork the code and change whatever you like, but the burden of maintaining your fork falls on you not the contributors of Bitcoin Core.
The contributors and builders of the bitcoin network do have a responsibility towards helping decentralise the network, which leads to overall network health.
In my humble opinion, helping support more diversity in client choice for node runners should be, if not already is, a core principle of the network.
Also this leads to less hardc
...
💬 maflcko commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3162656637)
> Can we merge this, the builds seem fine
No, it didn't. The build failed and you haven't tested this yourself at all?
> However i cant run the drahbot build, if somebody could add the label for building, would be helpful
A passing CI (or build) is not sufficient for a merge. Especially here, because a full guix build (with signing) isn't run by any CI. The burden to explain the changes (and why they are safe and correct, and how to test them) is on the pull request author. Instead
...
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3162656637)
> Can we merge this, the builds seem fine
No, it didn't. The build failed and you haven't tested this yourself at all?
> However i cant run the drahbot build, if somebody could add the label for building, would be helpful
A passing CI (or build) is not sufficient for a merge. Especially here, because a full guix build (with signing) isn't run by any CI. The burden to explain the changes (and why they are safe and correct, and how to test them) is on the pull request author. Instead
...
💬 maflcko commented on pull request "cmake: Switch to generated `ts_files.cmake` file":
(https://github.com/bitcoin/bitcoin/pull/33115#issuecomment-3162661955)
> The changes in `src/qt/bitcoin_szl.ts` look like they contain Rust source code?
Yes, see also https://github.com/maflcko/b-c-gui-translations-review/blob/fbeee68d9e07215e3ab0dc2e17ddf41ed9af7ce5/reviews/szl.md
(https://github.com/bitcoin/bitcoin/pull/33115#issuecomment-3162661955)
> The changes in `src/qt/bitcoin_szl.ts` look like they contain Rust source code?
Yes, see also https://github.com/maflcko/b-c-gui-translations-review/blob/fbeee68d9e07215e3ab0dc2e17ddf41ed9af7ce5/reviews/szl.md
💬 maflcko commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#issuecomment-3162681579)
> So this is obviously faster for many tiny tests, will run the same for the full test suite and do a full code review later.
Nice. Thanks for testing and confirming that the code without any sleep is the fastest of the 3 versions. I think benchmarking the full test suite may be difficult, because it has internal noise higher than the savings here. At least back in https://github.com/bitcoin/bitcoin/pull/13384#issuecomment-394335449, it was hard to measure, but maybe there is less noise now a
...
(https://github.com/bitcoin/bitcoin/pull/33141#issuecomment-3162681579)
> So this is obviously faster for many tiny tests, will run the same for the full test suite and do a full code review later.
Nice. Thanks for testing and confirming that the code without any sleep is the fastest of the 3 versions. I think benchmarking the full test suite may be difficult, because it has internal noise higher than the savings here. At least back in https://github.com/bitcoin/bitcoin/pull/13384#issuecomment-394335449, it was hard to measure, but maybe there is less noise now a
...
💬 optout21 commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#issuecomment-3162698716)
LGTM!
Minor comments:
- Maybe add a note to the `ComputeMerkleRoot` declaration, that the `hashes` input is expected to have one extra capacity (in case size is odd). Minor, as this function is not used directly much.
- Even more, it could debug-assert that capacity is even, but this may be a bit of a stretch.
(https://github.com/bitcoin/bitcoin/pull/32497#issuecomment-3162698716)
LGTM!
Minor comments:
- Maybe add a note to the `ComputeMerkleRoot` declaration, that the `hashes` input is expected to have one extra capacity (in case size is odd). Minor, as this function is not used directly much.
- Even more, it could debug-assert that capacity is even, but this may be a bit of a stretch.
🤔 maflcko reviewed a pull request: "Add Docker support with multi-arch build and user permissions handling"
(https://github.com/bitcoin/bitcoin/pull/33139#pullrequestreview-3095800749)
now you are just copy-pasting random files from the internet without even mentioning the source?
(https://github.com/bitcoin/bitcoin/pull/33139#pullrequestreview-3095800749)
now you are just copy-pasting random files from the internet without even mentioning the source?
💬 maflcko commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#discussion_r2259396974)
copy-paste from https://github.com/hundehausen/tor-hidden-service-docker/blob/main/Dockerfile
(https://github.com/bitcoin/bitcoin/pull/33139#discussion_r2259396974)
copy-paste from https://github.com/hundehausen/tor-hidden-service-docker/blob/main/Dockerfile
💬 maflcko commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#discussion_r2259458592)
This needs a proper workflow documented on how to bump it and when. Otherwise, it would be confusing to have a 30.0 git release tag which contains a dockerfile with a hard-coded 29.0 tag.
(https://github.com/bitcoin/bitcoin/pull/33139#discussion_r2259458592)
This needs a proper workflow documented on how to bump it and when. Otherwise, it would be confusing to have a 30.0 git release tag which contains a dockerfile with a hard-coded 29.0 tag.
💬 maflcko commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#discussion_r2259402491)
copy-paste from https://github.com/willcl-ark/bitcoin-core-docker/blob/master/29.0/alpine/Dockerfile
(https://github.com/bitcoin/bitcoin/pull/33139#discussion_r2259402491)
copy-paste from https://github.com/willcl-ark/bitcoin-core-docker/blob/master/29.0/alpine/Dockerfile
✅ fanquake closed a pull request: "doc:Fix typos"
(https://github.com/bitcoin/bitcoin/pull/33148)
(https://github.com/bitcoin/bitcoin/pull/33148)
💬 fanquake commented on pull request "doc:Fix typos":
(https://github.com/bitcoin/bitcoin/pull/33148#issuecomment-3162996627)
PIcked the relevant change into #33125.
(https://github.com/bitcoin/bitcoin/pull/33148#issuecomment-3162996627)
PIcked the relevant change into #33125.
💬 fanquake commented on pull request "ci: Use mlc `v1` and fix typos":
(https://github.com/bitcoin/bitcoin/pull/33125#discussion_r2259479844)
Dropped this for now.
(https://github.com/bitcoin/bitcoin/pull/33125#discussion_r2259479844)
Dropped this for now.
💬 maflcko commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3163010102)
For reference, for development, the dockerfile should look more like https://github.com/fanquake/core-review/blob/0bb70124a23807e3638f29ebd76c2bebc4880fd7/podman/debian.imagefile, just containing the install commands copy-pasted from the docs. But the development and deployment workflows differ significantly enough to not mix them up as the same.
My recommendation would be to just pick *one* problem/goal and not try to interleave multiple in the same pull request.
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3163010102)
For reference, for development, the dockerfile should look more like https://github.com/fanquake/core-review/blob/0bb70124a23807e3638f29ebd76c2bebc4880fd7/podman/debian.imagefile, just containing the install commands copy-pasted from the docs. But the development and deployment workflows differ significantly enough to not mix them up as the same.
My recommendation would be to just pick *one* problem/goal and not try to interleave multiple in the same pull request.