💬 fanquake commented on pull request "[22.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/26927#issuecomment-1433287884)
Going to merge this to unbreak building with GCC 13.x. i.e on rawhide.
(https://github.com/bitcoin/bitcoin/pull/26927#issuecomment-1433287884)
Going to merge this to unbreak building with GCC 13.x. i.e on rawhide.
💬 jamesob commented on pull request "OP_VAULT draft":
(https://github.com/bitcoin/bitcoin/pull/26857#discussion_r1108665794)
Good cleanup, done - thanks.
(https://github.com/bitcoin/bitcoin/pull/26857#discussion_r1108665794)
Good cleanup, done - thanks.
💬 hebasto commented on pull request "build: Check usages of #if defined(...)":
(https://github.com/bitcoin/bitcoin/pull/25302#issuecomment-1433291262)
Another bug found, which needs to be addressed.
The following code
https://github.com/bitcoin/bitcoin/blob/75f0e0b607cd7ff7afd56853eb34a2b285b22ad2/configure.ac#L1251-L1259
always defines the `HAVE_STRONG_GETAUXVAL` macro.
So the following `#if defined` directives can't handle it properly:
https://github.com/bitcoin/bitcoin/blob/75f0e0b607cd7ff7afd56853eb34a2b285b22ad2/src/randomenv.cpp#L56-L58 and https://github.com/bitcoin/bitcoin/blob/75f0e0b607cd7ff7afd56853eb34a2b285b22ad2/src/rand
...
(https://github.com/bitcoin/bitcoin/pull/25302#issuecomment-1433291262)
Another bug found, which needs to be addressed.
The following code
https://github.com/bitcoin/bitcoin/blob/75f0e0b607cd7ff7afd56853eb34a2b285b22ad2/configure.ac#L1251-L1259
always defines the `HAVE_STRONG_GETAUXVAL` macro.
So the following `#if defined` directives can't handle it properly:
https://github.com/bitcoin/bitcoin/blob/75f0e0b607cd7ff7afd56853eb34a2b285b22ad2/src/randomenv.cpp#L56-L58 and https://github.com/bitcoin/bitcoin/blob/75f0e0b607cd7ff7afd56853eb34a2b285b22ad2/src/rand
...
🚀 fanquake merged a pull request: "[22.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/26927)
(https://github.com/bitcoin/bitcoin/pull/26927)
💬 jamesob commented on pull request "fix: contrib: allow multi-sig binary verification":
(https://github.com/bitcoin/bitcoin/pull/23020#issuecomment-1433301398)
> We either need to make these scripts useful/usable, or we should just delete them.
Agreed - right now in master this script is useless for contemporary versions.
Unless anyone thinks otherwise, I think the best way forward here is for me to remove support for pre-multi-sig binaries and compress this change down to a single commit for easier review.
(https://github.com/bitcoin/bitcoin/pull/23020#issuecomment-1433301398)
> We either need to make these scripts useful/usable, or we should just delete them.
Agreed - right now in master this script is useless for contemporary versions.
Unless anyone thinks otherwise, I think the best way forward here is for me to remove support for pre-multi-sig binaries and compress this change down to a single commit for easier review.
💬 MarcoFalke commented on pull request "validation: Improve error handling when VerifyDB fails due to insufficient dbcache":
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1108687736)
Maybe just drop the hunk? Shouldn't matter either way :man_shrugging:
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1108687736)
Maybe just drop the hunk? Shouldn't matter either way :man_shrugging:
💬 fanquake commented on pull request "[23.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/26921#issuecomment-1433314002)
Also merging this to unbreak the GCC 13 build.
(https://github.com/bitcoin/bitcoin/pull/26921#issuecomment-1433314002)
Also merging this to unbreak the GCC 13 build.
💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#issuecomment-1433315562)
Thanks for the review. [diff](https://github.com/bitcoin/bitcoin/compare/f54210f50ec0cd44f026988308d975397a948d8c..1f8c242e07a0842a3c7cc8735a6dde3e92327bb4)
(https://github.com/bitcoin/bitcoin/pull/26177#issuecomment-1433315562)
Thanks for the review. [diff](https://github.com/bitcoin/bitcoin/compare/f54210f50ec0cd44f026988308d975397a948d8c..1f8c242e07a0842a3c7cc8735a6dde3e92327bb4)
🚀 fanquake merged a pull request: "[23.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/26921)
(https://github.com/bitcoin/bitcoin/pull/26921)
💬 glozow commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1433329273)
I pushed some changes (with @Xekyo's permission, thanks):
- Capped traversal at 500 items in `CalculateCluster()`. Number is arbitrary, open for commentary
- Fixed up a few things in the MiniMiner implementation, mostly shuffling things around and updating comments
- Dropped the chain interface changes (I think those can go in #26152)
- Expanded unit tests
- Added a fuzzer (expanded from @dergoegge's, thanks)
- Added a fuzzer to differentially test block templates built by `BlockAssembler`
...
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1433329273)
I pushed some changes (with @Xekyo's permission, thanks):
- Capped traversal at 500 items in `CalculateCluster()`. Number is arbitrary, open for commentary
- Fixed up a few things in the MiniMiner implementation, mostly shuffling things around and updating comments
- Dropped the chain interface changes (I think those can go in #26152)
- Expanded unit tests
- Added a fuzzer (expanded from @dergoegge's, thanks)
- Added a fuzzer to differentially test block templates built by `BlockAssembler`
...
💬 pinheadmz commented on pull request "system: use %LOCALAPPDATA% as default datadir on windows":
(https://github.com/bitcoin/bitcoin/pull/27064#issuecomment-1433331637)
Would this change need a `release-notes-27064.md` file? It is kinda changing a configuration model
(https://github.com/bitcoin/bitcoin/pull/27064#issuecomment-1433331637)
Would this change need a `release-notes-27064.md` file? It is kinda changing a configuration model
📝 TheCharlatan review_request_removed a pull request: "refactor / kernel: Move non-gArgs chainparams functionality to kernel"
(https://github.com/bitcoin/bitcoin/pull/26177)
This pull request is mostly a code-move of chainparams related functionality into the kernel library. @dongcarl is the original author of this patchset, these commits were taken from https://github.com/dongcarl/bitcoin/tree/2022-03-libbitcoinkernel-chainparams-args-only.
#### Context
The bitcoin kernel library currently relies on code containing user configurations through the `ArgsManager`. This is not optimal, since as a stand-alone library it should not rely on most of the user-define
...
(https://github.com/bitcoin/bitcoin/pull/26177)
This pull request is mostly a code-move of chainparams related functionality into the kernel library. @dongcarl is the original author of this patchset, these commits were taken from https://github.com/dongcarl/bitcoin/tree/2022-03-libbitcoinkernel-chainparams-args-only.
#### Context
The bitcoin kernel library currently relies on code containing user configurations through the `ArgsManager`. This is not optimal, since as a stand-alone library it should not rely on most of the user-define
...
📝 TheCharlatan review_request_removed a pull request: "refactor / kernel: Move non-gArgs chainparams functionality to kernel"
(https://github.com/bitcoin/bitcoin/pull/26177)
This pull request is mostly a code-move of chainparams related functionality into the kernel library. @dongcarl is the original author of this patchset, these commits were taken from https://github.com/dongcarl/bitcoin/tree/2022-03-libbitcoinkernel-chainparams-args-only.
#### Context
The bitcoin kernel library currently relies on code containing user configurations through the `ArgsManager`. This is not optimal, since as a stand-alone library it should not rely on most of the user-define
...
(https://github.com/bitcoin/bitcoin/pull/26177)
This pull request is mostly a code-move of chainparams related functionality into the kernel library. @dongcarl is the original author of this patchset, these commits were taken from https://github.com/dongcarl/bitcoin/tree/2022-03-libbitcoinkernel-chainparams-args-only.
#### Context
The bitcoin kernel library currently relies on code containing user configurations through the `ArgsManager`. This is not optimal, since as a stand-alone library it should not rely on most of the user-define
...
💬 MarcoFalke commented on pull request "lint: enable E722 do not use bare except":
(https://github.com/bitcoin/bitcoin/pull/25867#issuecomment-1433342403)
> bare excepts should be replaced with specific exceptions, not except Exception.
I wonder if this should be done.
At least it is unclear to me how to review this easily. And if it is hard to review and maintain, I wonder if it is worth it.
(https://github.com/bitcoin/bitcoin/pull/25867#issuecomment-1433342403)
> bare excepts should be replaced with specific exceptions, not except Exception.
I wonder if this should be done.
At least it is unclear to me how to review this easily. And if it is hard to review and maintain, I wonder if it is worth it.
💬 hebasto commented on pull request "src/randomenv.cpp: fix build on uclibc":
(https://github.com/bitcoin/bitcoin/pull/20358#issuecomment-1433349312)
Hmm, this patch looks [buggy](https://github.com/bitcoin/bitcoin/pull/25302#issuecomment-1433291262)...
(https://github.com/bitcoin/bitcoin/pull/20358#issuecomment-1433349312)
Hmm, this patch looks [buggy](https://github.com/bitcoin/bitcoin/pull/25302#issuecomment-1433291262)...
📝 MarcoFalke unlocked a pull request: "src/randomenv.cpp: fix build on uclibc"
(https://github.com/bitcoin/bitcoin/pull/20358)
Check for HAVE_STRONG_GETAUXVAL or HAVE_WEAK_GETAUXVAL before using
getauxval to avoid a build failure on uclibc
Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
(https://github.com/bitcoin/bitcoin/pull/20358)
Check for HAVE_STRONG_GETAUXVAL or HAVE_WEAK_GETAUXVAL before using
getauxval to avoid a build failure on uclibc
Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
💬 achow101 commented on pull request "refactor: Move coin_control variable to test setup section":
(https://github.com/bitcoin/bitcoin/pull/26154#issuecomment-1433353547)
> Tiny PRs like this in my mind should be reviewed and either closed or merged in a short period since it's trivial to review. I'm surprised it's taken 6 months to even be looked at.
The fact that a tiny PR like this one has taken 6 months for anyone to even look at it is why I think it is noise. There are over 300 other PRs to look at, with several large projects going on at the same time. People are busy looking at other things, and simply having this PR even exist means that time that coul
...
(https://github.com/bitcoin/bitcoin/pull/26154#issuecomment-1433353547)
> Tiny PRs like this in my mind should be reviewed and either closed or merged in a short period since it's trivial to review. I'm surprised it's taken 6 months to even be looked at.
The fact that a tiny PR like this one has taken 6 months for anyone to even look at it is why I think it is noise. There are over 300 other PRs to look at, with several large projects going on at the same time. People are busy looking at other things, and simply having this PR even exist means that time that coul
...
💬 MarcoFalke commented on pull request "refactor: Move coin_control variable to test setup section":
(https://github.com/bitcoin/bitcoin/pull/26154#issuecomment-1433359488)
I also have a template response:
Thank you for your contribution. While this stylistic change makes sense on its own, it comes at a cost and risk for the project as a whole. The weak motivation for the change does not justify the burden that it places on the project. A burden could be any of the following:
* Time spent on review
* Accidental introduction of bugs
* (Silent) merge conflicts, either in the branch or a backport branch. Those conflicts demand further developer and reviewer
...
(https://github.com/bitcoin/bitcoin/pull/26154#issuecomment-1433359488)
I also have a template response:
Thank you for your contribution. While this stylistic change makes sense on its own, it comes at a cost and risk for the project as a whole. The weak motivation for the change does not justify the burden that it places on the project. A burden could be any of the following:
* Time spent on review
* Accidental introduction of bugs
* (Silent) merge conflicts, either in the branch or a backport branch. Those conflicts demand further developer and reviewer
...
⚠️ MarcoFalke opened an issue: "Intermittent issue in p2p_ibd_stalling.py self.wait_until(lambda: self.total_bytes_recv_for_blocks() == 172761)"
(https://github.com/bitcoin/bitcoin/issues/27112)
```
wget https://drahtbot.space/temp_scratch/p2p_ibd_stalling_96.tar.xz
tar -xvf p2p_ibd_stalling_96.tar.xz
test/functional/combine_logs.py -c ./p2p_ibd_stalling_96
```
```
test 2023-02-14T20:54:45.479000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/root/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 134, in
...
(https://github.com/bitcoin/bitcoin/issues/27112)
```
wget https://drahtbot.space/temp_scratch/p2p_ibd_stalling_96.tar.xz
tar -xvf p2p_ibd_stalling_96.tar.xz
test/functional/combine_logs.py -c ./p2p_ibd_stalling_96
```
```
test 2023-02-14T20:54:45.479000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/root/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 134, in
...
📝 achow101 opened a pull request: "rpc: Use a FlatSigningProvider in decodescript to allow inferring descriptors for scripts larger than 520 bytes"
(https://github.com/bitcoin/bitcoin/pull/27113)
`FillableSigningProvider` limits scripts to 520 bytes even though segwit allows scripts to be larger than that. We can avoid this limit by using a `FlatSigningProvider` so that such larger scripts can be decoded.
Fixes #27111
(https://github.com/bitcoin/bitcoin/pull/27113)
`FillableSigningProvider` limits scripts to 520 bytes even though segwit allows scripts to be larger than that. We can avoid this limit by using a `FlatSigningProvider` so that such larger scripts can be decoded.
Fixes #27111