💬 pablomartin4btc commented on pull request "test: Check already deactivated network stays suspended after dumptxoutset":
(https://github.com/bitcoin/bitcoin/pull/30892#discussion_r1758531772)
I can open a new PR to test the rollback type option with both a valid and invalid heights. I'll do as soon as this one gets merged. Thanks.
(https://github.com/bitcoin/bitcoin/pull/30892#discussion_r1758531772)
I can open a new PR to test the rollback type option with both a valid and invalid heights. I'll do as soon as this one gets merged. Thanks.
💬 hebasto commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2348497540)
> 20 - 75s is waiting for these to complete:
>
> 133/134 Test #5: noverify_tests ....................... Passed 36.41 sec
> 134/134 Test #6: tests ................................ Passed 76.79 sec
This part is addressed in https://github.com/bitcoin-core/secp256k1/pull/1581.
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2348497540)
> 20 - 75s is waiting for these to complete:
>
> 133/134 Test #5: noverify_tests ....................... Passed 36.41 sec
> 134/134 Test #6: tests ................................ Passed 76.79 sec
This part is addressed in https://github.com/bitcoin-core/secp256k1/pull/1581.
💬 fjahr commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1758554116)
More or a question: when `check_for_undo` is true doe we actually want to check for both? Having undo and not data set here would seem to be something going weirdly wrong though I guess.
```suggestion
uint32_t flag = check_for_undo ? BLOCK_HAVE_MASK : BLOCK_HAVE_DATA;
```
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1758554116)
More or a question: when `check_for_undo` is true doe we actually want to check for both? Having undo and not data set here would seem to be something going weirdly wrong though I guess.
```suggestion
uint32_t flag = check_for_undo ? BLOCK_HAVE_MASK : BLOCK_HAVE_DATA;
```
💬 maflcko commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2348524416)
> I have noticed this in the cirrus GUI before, it seems to have always been like this though IIRC:
Yeah, I think the graph there are not accurate and a bit misleading. It may be best to ignore them. I used a `CPX51` Hetzner Box with 16 vCPU, which should be enough to run any CI task with a completely empty cache. I just looked at the CPU/IO usage in the Cloud Console Graph.
> Perhaps these shared machines are hitting other IO limits? Doesn't seems to be much other explanation for it. Out
...
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2348524416)
> I have noticed this in the cirrus GUI before, it seems to have always been like this though IIRC:
Yeah, I think the graph there are not accurate and a bit misleading. It may be best to ignore them. I used a `CPX51` Hetzner Box with 16 vCPU, which should be enough to run any CI task with a completely empty cache. I just looked at the CPU/IO usage in the Cloud Console Graph.
> Perhaps these shared machines are hitting other IO limits? Doesn't seems to be much other explanation for it. Out
...
🤔 pablomartin4btc reviewed a pull request: "doc: unit test runner help fixup"
(https://github.com/bitcoin/bitcoin/pull/30890#pullrequestreview-2302673637)
ACK 0024d2c6ea0fafe9b9949af4bbcd0c583e580746
nit: maybe you can add "Run `test_bitcoin --list_content` for the full list of tests." in the previous section "**Compiling/running unit tests**", before the paragraph starting with "_To run the unit tests manually,.._".
(https://github.com/bitcoin/bitcoin/pull/30890#pullrequestreview-2302673637)
ACK 0024d2c6ea0fafe9b9949af4bbcd0c583e580746
nit: maybe you can add "Run `test_bitcoin --list_content` for the full list of tests." in the previous section "**Compiling/running unit tests**", before the paragraph starting with "_To run the unit tests manually,.._".
💬 fanquake commented on pull request "doc: unit test runner help fixup":
(https://github.com/bitcoin/bitcoin/pull/30890#issuecomment-2348531789)
Can probably squash, given the single line changed in the first commit, is then deleted/moved in the second commit.
(https://github.com/bitcoin/bitcoin/pull/30890#issuecomment-2348531789)
Can probably squash, given the single line changed in the first commit, is then deleted/moved in the second commit.
🤔 fjahr reviewed a pull request: "rpc, rest: Improve block rpc error handling, check header before attempting to read block data."
(https://github.com/bitcoin/bitcoin/pull/30410#pullrequestreview-2302677474)
Code review ACK a5df908d981138f066f5f963a1813d453483814a
Feel free to ignore typo comments unless you retouch.
Also saw a typo in 8bfb0bd7bd5f8963fb1f58988f9bb36199dc98de: "dowloaded"
(https://github.com/bitcoin/bitcoin/pull/30410#pullrequestreview-2302677474)
Code review ACK a5df908d981138f066f5f963a1813d453483814a
Feel free to ignore typo comments unless you retouch.
Also saw a typo in 8bfb0bd7bd5f8963fb1f58988f9bb36199dc98de: "dowloaded"
💬 fjahr commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1758559803)
typo: corruption
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1758559803)
typo: corruption
💬 fjahr commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1758560350)
typo: corruption
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1758560350)
typo: corruption
💬 fjahr commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1758560653)
typo: corruption
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1758560653)
typo: corruption
💬 fanquake commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2348545844)
Ping also @mzumsande, given you [expressed some reservations](https://github.com/bitcoin/bitcoin/pull/28043#pullrequestreview-1846077000) to this approach last time.
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2348545844)
Ping also @mzumsande, given you [expressed some reservations](https://github.com/bitcoin/bitcoin/pull/28043#pullrequestreview-1846077000) to this approach last time.
💬 maflcko commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2348550239)
> network IO
Reminds me that Cirrus Logs are randomly disappearing. So maybe there is an issue in the Cirrus backend that streams logs and the streaming process dies or turns into a zombie process?
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2348550239)
> network IO
Reminds me that Cirrus Logs are randomly disappearing. So maybe there is an issue in the Cirrus backend that streams logs and the streaming process dies or turns into a zombie process?
💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1758574372)
Seems like there is still some tinkering going on on that one, I will give it a try when that has settled down.
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1758574372)
Seems like there is still some tinkering going on on that one, I will give it a try when that has settled down.
🤔 hebasto reviewed a pull request: "Fix crash when closing wallet"
(https://github.com/bitcoin-core/gui/pull/835#pullrequestreview-2302703900)
Concept ACK.
`git bisect` points to 5d15485aafefdc759ba97e039bb1b9ccac267358 from https://github.com/bitcoin/bitcoin/pull/30659 as the root of the issue, which aligns with the other [comment](https://github.com/bitcoin/bitcoin/issues/30887#issuecomment-2347237302).
It would be reasonable to add a dedicated test to `test_bitcoin-qt` in a follow-up.
(https://github.com/bitcoin-core/gui/pull/835#pullrequestreview-2302703900)
Concept ACK.
`git bisect` points to 5d15485aafefdc759ba97e039bb1b9ccac267358 from https://github.com/bitcoin/bitcoin/pull/30659 as the root of the issue, which aligns with the other [comment](https://github.com/bitcoin/bitcoin/issues/30887#issuecomment-2347237302).
It would be reasonable to add a dedicated test to `test_bitcoin-qt` in a follow-up.
🤔 pablomartin4btc reviewed a pull request: "doc: replaced --enable-debug with -DCMAKE_BUILD_TYPE=Debug in developer-notes"
(https://github.com/bitcoin/bitcoin/pull/30875#pullrequestreview-2302713059)
ACK dd0941cc6de6852c8ee1347c7ccfcc887cd07e4b
nit: since you are there at `doc/zmq.md`, I think there's a typo close to the end, look for "chain--as".
(https://github.com/bitcoin/bitcoin/pull/30875#pullrequestreview-2302713059)
ACK dd0941cc6de6852c8ee1347c7ccfcc887cd07e4b
nit: since you are there at `doc/zmq.md`, I think there's a typo close to the end, look for "chain--as".
⚠️ Abbbyrose opened an issue: "Hrlp me reach my goal of 10k"
(https://github.com/bitcoin/bitcoin/issues/30895)
https://cash.app/launch/bitcoin/$abbyyrosemckenna/Fs1ntjfabE
(https://github.com/bitcoin/bitcoin/issues/30895)
https://cash.app/launch/bitcoin/$abbyyrosemckenna/Fs1ntjfabE
💬 maflcko commented on pull request "guix: Drop unused autotools packages":
(https://github.com/bitcoin/bitcoin/pull/30752#discussion_r1758594864)
If libtool is fine to drop here in guix, which uses depends, why is it not dropped from the depends docs as well, and the CI tasks?
```
depends/README.md: apt install automake bison cmake curl libtool make patch pkg-config python3 xz-utils
depends/packages.md:In general, the output of a depends package should not contain any libtool
.github/workflows/ci.yml: brew install --quiet automake libtool pkg-config gnu-getopt ccache boost libevent miniupnpc libnatpmp zeromq qt@5 qrencod
...
(https://github.com/bitcoin/bitcoin/pull/30752#discussion_r1758594864)
If libtool is fine to drop here in guix, which uses depends, why is it not dropped from the depends docs as well, and the CI tasks?
```
depends/README.md: apt install automake bison cmake curl libtool make patch pkg-config python3 xz-utils
depends/packages.md:In general, the output of a depends package should not contain any libtool
.github/workflows/ci.yml: brew install --quiet automake libtool pkg-config gnu-getopt ccache boost libevent miniupnpc libnatpmp zeromq qt@5 qrencod
...
✅ fanquake closed an issue: "Help me reach my goal of 10k!!!"
(https://github.com/bitcoin/bitcoin/issues/30895)
(https://github.com/bitcoin/bitcoin/issues/30895)
:lock: fanquake locked an issue: "Help me reach my goal of 10k!!!"
(https://github.com/bitcoin/bitcoin/issues/30895)
(https://github.com/bitcoin/bitcoin/issues/30895)
💬 maflcko commented on pull request "doc: replaced --enable-debug with -DCMAKE_BUILD_TYPE=Debug in developer-notes":
(https://github.com/bitcoin/bitcoin/pull/30875#issuecomment-2348584112)
I think you are still missing:
```
test/lint/lint-spelling.py:FILES_ARGS = ['git', 'ls-files', '--', ":(exclude)build-aux/m4/", ":(exclude)contrib/seeds/*.txt", ":(exclude)depends/", ":(exclude)doc/release-notes/", ":(exclude)src/qt/locale/", ":(exclude)src/qt/*.qrc", ":(exclude)contrib/guix/patches"]
```
Also, the pull title should be updated?
(https://github.com/bitcoin/bitcoin/pull/30875#issuecomment-2348584112)
I think you are still missing:
```
test/lint/lint-spelling.py:FILES_ARGS = ['git', 'ls-files', '--', ":(exclude)build-aux/m4/", ":(exclude)contrib/seeds/*.txt", ":(exclude)depends/", ":(exclude)doc/release-notes/", ":(exclude)src/qt/locale/", ":(exclude)src/qt/*.qrc", ":(exclude)contrib/guix/patches"]
```
Also, the pull title should be updated?