💬 maflcko commented on pull request "test: XORed blocks test follow up":
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1730300469)
style nit, while touching this line: The argument is of type Path, so I think you can just call `unlink()` on it to avoid the `os` import?
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1730300469)
style nit, while touching this line: The argument is of type Path, so I think you can just call `unlink()` on it to avoid the `os` import?
💬 l0rinc commented on pull request "doc: fix 3 simple CI codespell warnings":
(https://github.com/bitcoin/bitcoin/pull/30700#issuecomment-2308765191)
> Probably preferable to avoid needless warnings
Since we seem to ignore many other QT related files and folder, I've added this one to the excludes - what do you think?
(https://github.com/bitcoin/bitcoin/pull/30700#issuecomment-2308765191)
> Probably preferable to avoid needless warnings
Since we seem to ignore many other QT related files and folder, I've added this one to the excludes - what do you think?
🤔 fjahr reviewed a pull request: "devtools, utxo-snapshot: Fix block height out of range in script"
(https://github.com/bitcoin/bitcoin/pull/30690#pullrequestreview-2259216416)
tACK 5b4f34006dbd76223b55b156421f87d2241ac296
Reviewed the code and did some light testing.
It's unclear when #29553 will get merged and since we may see some people come in and try out assumeutxo for the first time when v28 is released so I think it's ok to keep improving the script until then.
This could have also been fixed in a simpler way by moving the `PIVOT_BLOCKHASH=` line below the two `trap` lines but I guess it's nice to give users an understandable error message so I am fine
...
(https://github.com/bitcoin/bitcoin/pull/30690#pullrequestreview-2259216416)
tACK 5b4f34006dbd76223b55b156421f87d2241ac296
Reviewed the code and did some light testing.
It's unclear when #29553 will get merged and since we may see some people come in and try out assumeutxo for the first time when v28 is released so I think it's ok to keep improving the script until then.
This could have also been fixed in a simpler way by moving the `PIVOT_BLOCKHASH=` line below the two `trap` lines but I guess it's nice to give users an understandable error message so I am fine
...
💬 fjahr commented on pull request "devtools, utxo-snapshot: Fix block height out of range in script":
(https://github.com/bitcoin/bitcoin/pull/30690#discussion_r1730310284)
nit: Could reuse that below in the `PIVOT_BLOCKHASH=` line but only if you have to retouch.
(https://github.com/bitcoin/bitcoin/pull/30690#discussion_r1730310284)
nit: Could reuse that below in the `PIVOT_BLOCKHASH=` line but only if you have to retouch.
💬 l0rinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2308803683)
> Comparing the `objdump` of `sigopcount_tests.cpp.o` and `validation_chainstate_tests.cpp.o` revealed that for some reason ccache returned the content of `sigopcount_tests` for `validation_chainstate_tests` (exact match, 0 difference between the two).
I investigated further, and if I understand correctly, it appears that `ccache` is indeed returning `sigopcount_tests` binaries for `validation_chainstate_tests`.
I generated build debug logs using the following commands:
```bash
export CC
...
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2308803683)
> Comparing the `objdump` of `sigopcount_tests.cpp.o` and `validation_chainstate_tests.cpp.o` revealed that for some reason ccache returned the content of `sigopcount_tests` for `validation_chainstate_tests` (exact match, 0 difference between the two).
I investigated further, and if I understand correctly, it appears that `ccache` is indeed returning `sigopcount_tests` binaries for `validation_chainstate_tests`.
I generated build debug logs using the following commands:
```bash
export CC
...
📝 maflcko opened a pull request: " fuzz: Add missing fuzz targets to cmake build "
(https://github.com/bitcoin/bitcoin/pull/30712)
Fixes https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1726881676
Can be tested via:
```
PRINT_ALL_FUZZ_TARGETS_AND_ABORT=1 ./bld-autot/src/test/fuzz/fuzz > /tmp/f_autot
PRINT_ALL_FUZZ_TARGETS_AND_ABORT=1 ./bld-cmake/src/test/fuzz/fuzz > /tmp/f_cmake
diff --unified /tmp/{f_autot,f_cmake}
(https://github.com/bitcoin/bitcoin/pull/30712)
Fixes https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1726881676
Can be tested via:
```
PRINT_ALL_FUZZ_TARGETS_AND_ABORT=1 ./bld-autot/src/test/fuzz/fuzz > /tmp/f_autot
PRINT_ALL_FUZZ_TARGETS_AND_ABORT=1 ./bld-cmake/src/test/fuzz/fuzz > /tmp/f_cmake
diff --unified /tmp/{f_autot,f_cmake}
💬 tdb3 commented on pull request "test: XORed blocks test follow up":
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1730330695)
Thanks, that's cleaner. Added.
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1730330695)
Thanks, that's cleaner. Added.
🤔 Safay99 reviewed a pull request: "test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds"
(https://github.com/bitcoin/bitcoin/pull/29357#pullrequestreview-2259263018)
1. بیبی
(https://github.com/bitcoin/bitcoin/pull/29357#pullrequestreview-2259263018)
1. بیبی
💬 Safay99 commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-2308840524)
[
/](url)
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-2308840524)
[
/](url)
📝 tdb3 opened a pull request: "rpc: add revelant_blocks to scanblocks status"
(https://github.com/bitcoin/bitcoin/pull/30713)
Currently, after starting a scan with `scanblocks` the user must wait until the scan is complete to see the associated/relevant block hashes.
This updates `scanblocks status` results to provide the `relevant_blocks` found so far.
This enables earlier subsequent lookup through matching blocks (e.g. to run `getdescriptoractivity` in PR #30708)
Leaving this one as a (rough) draft for now. If enough concept ACK/NACK is received, then can look into adding tests (into `rpc_scanblocks.py`. `-dep
...
(https://github.com/bitcoin/bitcoin/pull/30713)
Currently, after starting a scan with `scanblocks` the user must wait until the scan is complete to see the associated/relevant block hashes.
This updates `scanblocks status` results to provide the `relevant_blocks` found so far.
This enables earlier subsequent lookup through matching blocks (e.g. to run `getdescriptoractivity` in PR #30708)
Leaving this one as a (rough) draft for now. If enough concept ACK/NACK is received, then can look into adding tests (into `rpc_scanblocks.py`. `-dep
...
💬 tdb3 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#issuecomment-2308842097)
> One possibility (that is probably more valuable) is to add an incremental `relevant_blocks` key to `scanblocks status` output, so that the client can begin calling `getdescriptoractivity` progressively as results roll in but before the entire scan is finished.
Great idea. Created an initial draft PR #30713
(https://github.com/bitcoin/bitcoin/pull/30708#issuecomment-2308842097)
> One possibility (that is probably more valuable) is to add an incremental `relevant_blocks` key to `scanblocks status` output, so that the client can begin calling `getdescriptoractivity` progressively as results roll in but before the entire scan is finished.
Great idea. Created an initial draft PR #30713
💬 Albroheem commented on issue "Release Schedule for 28.0":
(https://github.com/bitcoin/bitcoin/issues/29891#issuecomment-2308888456)
tar xf bitcoin-osx-unsigned.tar.gz
./detached-sig-create.sh /path/to/codesign.p12
Enter the keychain password and authorize the signature
signature-osx.tar.gz will be created
(https://github.com/bitcoin/bitcoin/issues/29891#issuecomment-2308888456)
tar xf bitcoin-osx-unsigned.tar.gz
./detached-sig-create.sh /path/to/codesign.p12
Enter the keychain password and authorize the signature
signature-osx.tar.gz will be created
💬 hebasto commented on pull request "fuzz: Add missing fuzz targets to cmake build":
(https://github.com/bitcoin/bitcoin/pull/30712#issuecomment-2308897782)
Looks correct.
(https://github.com/bitcoin/bitcoin/pull/30712#issuecomment-2308897782)
Looks correct.
💬 TheCharlatan commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1730004982)
Nit (clang-format): Space after closing parenthesis.
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1730004982)
Nit (clang-format): Space after closing parenthesis.
🤔 TheCharlatan reviewed a pull request: "init: fixes file descriptor accounting"
(https://github.com/bitcoin/bitcoin/pull/30065#pullrequestreview-2258747309)
This looks good, but I think these few, simple things should be addressed.
(https://github.com/bitcoin/bitcoin/pull/30065#pullrequestreview-2258747309)
This looks good, but I think these few, simple things should be addressed.
💬 TheCharlatan commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1730402391)
nit: `static constexpr`
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1730402391)
nit: `static constexpr`
💬 TheCharlatan commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1730408130)
I think this should be de-globalized and renamed like I've done here: https://github.com/TheCharlatan/bitcoin/commit/a2c048322c67436e02a3a9607400f65f10dd6564
The `avaialble_fds` global variable there are also kind of silly, since it is only there to be logged at a later point in time, but I don't feel strongly if it should go or not either way.
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1730408130)
I think this should be de-globalized and renamed like I've done here: https://github.com/TheCharlatan/bitcoin/commit/a2c048322c67436e02a3a9607400f65f10dd6564
The `avaialble_fds` global variable there are also kind of silly, since it is only there to be logged at a later point in time, but I don't feel strongly if it should go or not either way.
💬 TheCharlatan commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1730402947)
Why did you not put `MAX_ADDNODE_CONNECTIONS` in here?
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1730402947)
Why did you not put `MAX_ADDNODE_CONNECTIONS` in here?
💬 pablomartin4btc commented on pull request "Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog":
(https://github.com/bitcoin-core/gui/pull/762#issuecomment-2308962681)
Updates:
- Addressed @hebasto's feedback.
- Fixed the [error message window inconsistency](https://github.com/bitcoin-core/gui/pull/762#issuecomment-2276259183) detected by @hebasto [above](https://github.com/bitcoin-core/gui/pull/762#pullrequestreview-2219281894).
<details>
<summary>Now running <code>qt</code> from the command line with <code>bitcoin-qt.exe -signet -about</code> (or similar command with an invalid argument) will show the correct chain type icon at the top left of the tit
...
(https://github.com/bitcoin-core/gui/pull/762#issuecomment-2308962681)
Updates:
- Addressed @hebasto's feedback.
- Fixed the [error message window inconsistency](https://github.com/bitcoin-core/gui/pull/762#issuecomment-2276259183) detected by @hebasto [above](https://github.com/bitcoin-core/gui/pull/762#pullrequestreview-2219281894).
<details>
<summary>Now running <code>qt</code> from the command line with <code>bitcoin-qt.exe -signet -about</code> (or similar command with an invalid argument) will show the correct chain type icon at the top left of the tit
...
📝 theStack opened a pull request: "test: fix `TestShell` initialization (late follow-up for #30463)"
(https://github.com/bitcoin/bitcoin/pull/30714)
Creating a `TestShell` instance as stated in the [docs](https://github.com/bitcoin/bitcoin/blob/master/test/functional/test-shell.md) currently fails on master:
```
$ python3
Python 3.10.13 (main, Mar 15 2024, 07:36:23) [Clang 16.0.6 ] on openbsd7
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.path.insert(0, "/home/thestack/bitcoin/test/functional")
>>> from test_framework.test_shell import TestShell
>>> test = TestShell().setup(num_nodes=2,
...
(https://github.com/bitcoin/bitcoin/pull/30714)
Creating a `TestShell` instance as stated in the [docs](https://github.com/bitcoin/bitcoin/blob/master/test/functional/test-shell.md) currently fails on master:
```
$ python3
Python 3.10.13 (main, Mar 15 2024, 07:36:23) [Clang 16.0.6 ] on openbsd7
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.path.insert(0, "/home/thestack/bitcoin/test/functional")
>>> from test_framework.test_shell import TestShell
>>> test = TestShell().setup(num_nodes=2,
...