💬 fjahr commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743875846)
I checked the developer notes on the definition again and I think you are right that a `LogWarning` is a better fit. I think it's not a "severe problem" necessarily but it does need to be addressed by the node admin if they want to run `dumptxoutset` successfully.
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743875846)
I checked the developer notes on the definition again and I think you are right that a `LogWarning` is a better fit. I think it's not a "severe problem" necessarily but it does need to be addressed by the node admin if they want to run `dumptxoutset` successfully.
💬 maflcko commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#issuecomment-2329170695)
re-ACK a3108a7c5692d137b70b8442b4741936277e89be 🐸
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK a3108a7c5692d137b70b
...
(https://github.com/bitcoin/bitcoin/pull/30808#issuecomment-2329170695)
re-ACK a3108a7c5692d137b70b8442b4741936277e89be 🐸
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK a3108a7c5692d137b70b
...
🚀 glozow merged a pull request: "test: add check that too large txs aren't put into orphanage"
(https://github.com/bitcoin/bitcoin/pull/30784)
(https://github.com/bitcoin/bitcoin/pull/30784)
💬 fanquake commented on pull request "depends: build libevent with `-D_GNU_SOURCE`":
(https://github.com/bitcoin/bitcoin/pull/30743#issuecomment-2329212785)
> EDIT: Probably something with the zipping again?
Yea, looks like a permissions issue:
#### Comparing `bitcoin-556775408797-arm64-apple-darwin-unsigned.zip.fanquake` & `bitcoin-556775408797-arm64-apple-darwin-unsigned.zip.thecharlatan`
#### zipinfo {}
```diff
@@ -1,18 +1,18 @@
Zip file size: 16309329 bytes, number of entries: 46
drwxr-xr-x 3.0 unx 0 b- stor 24-Aug-28 16:35 Bitcoin-Qt.app/
drwxr-xr-x 3.0 unx 0 b- stor 24-Aug-28 16:35 Bitcoin-Qt.app/Contents/
--
...
(https://github.com/bitcoin/bitcoin/pull/30743#issuecomment-2329212785)
> EDIT: Probably something with the zipping again?
Yea, looks like a permissions issue:
#### Comparing `bitcoin-556775408797-arm64-apple-darwin-unsigned.zip.fanquake` & `bitcoin-556775408797-arm64-apple-darwin-unsigned.zip.thecharlatan`
#### zipinfo {}
```diff
@@ -1,18 +1,18 @@
Zip file size: 16309329 bytes, number of entries: 46
drwxr-xr-x 3.0 unx 0 b- stor 24-Aug-28 16:35 Bitcoin-Qt.app/
drwxr-xr-x 3.0 unx 0 b- stor 24-Aug-28 16:35 Bitcoin-Qt.app/Contents/
--
...
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1743907527)
Yes, will fix if I retouch
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1743907527)
Yes, will fix if I retouch
💬 fjahr commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#issuecomment-2329225316)
Concept ACK
I've read through https://github.com/bitcoin/bitcoin/pull/30385 and this seems like a good change.
(https://github.com/bitcoin/bitcoin/pull/30807#issuecomment-2329225316)
Concept ACK
I've read through https://github.com/bitcoin/bitcoin/pull/30385 and this seems like a good change.
💬 fjahr commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#issuecomment-2329230808)
CI doesn't seem happy though. Kind of weird, the logs don't load for me.
(https://github.com/bitcoin/bitcoin/pull/30807#issuecomment-2329230808)
CI doesn't seem happy though. Kind of weird, the logs don't load for me.
💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#issuecomment-2329234757)
Follow-ups are here: https://github.com/bitcoin/bitcoin/pull/30808
(https://github.com/bitcoin/bitcoin/pull/29553#issuecomment-2329234757)
Follow-ups are here: https://github.com/bitcoin/bitcoin/pull/30808
💬 kevkevinpal commented on pull request "lint: Speed up and fix flake8 checks":
(https://github.com/bitcoin/bitcoin/pull/30723#issuecomment-2329240289)
ACK [fafdb7d](https://github.com/bitcoin/bitcoin/pull/30723/commits/fafdb7df34507eee735893aa871da6ae529e6372)
Removal of the unnecessary lint checks and the speed up of the checks are great updates!
(https://github.com/bitcoin/bitcoin/pull/30723#issuecomment-2329240289)
ACK [fafdb7d](https://github.com/bitcoin/bitcoin/pull/30723/commits/fafdb7df34507eee735893aa871da6ae529e6372)
Removal of the unnecessary lint checks and the speed up of the checks are great updates!
💬 furszy commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#issuecomment-2329262030)
> CI doesn't seem happy though. Kind of weird, the logs don't load for me.
Yeah, working on it. Almost there.
(https://github.com/bitcoin/bitcoin/pull/30807#issuecomment-2329262030)
> CI doesn't seem happy though. Kind of weird, the logs don't load for me.
Yeah, working on it. Almost there.
🤔 pablomartin4btc reviewed a pull request: "rpc: dumptxoutset height parameter follow-ups (29553)"
(https://github.com/bitcoin/bitcoin/pull/30808#pullrequestreview-2280450117)
cr ACK a3108a7c5692d137b70b8442b4741936277e89be
Thanks for addressing the 3 follow-ups. I'll test this soon.
Is it worth it to add a test to produce the situation where the reconsider didn't happen and the height stayed at the rollback/ invalidated height/ stale tip (test should fail in prod and not with this PR)?
<details>
<summary>Here some more details (rpc: <code>dumptxoutset</code>, <code>getchaintips</code> & <code>getcblockchaininfo</code>) about the situation described at the t
...
(https://github.com/bitcoin/bitcoin/pull/30808#pullrequestreview-2280450117)
cr ACK a3108a7c5692d137b70b8442b4741936277e89be
Thanks for addressing the 3 follow-ups. I'll test this soon.
Is it worth it to add a test to produce the situation where the reconsider didn't happen and the height stayed at the rollback/ invalidated height/ stale tip (test should fail in prod and not with this PR)?
<details>
<summary>Here some more details (rpc: <code>dumptxoutset</code>, <code>getchaintips</code> & <code>getcblockchaininfo</code>) about the situation described at the t
...
💬 marcofleon commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1743946931)
On my machine, running
`FUZZ=hex build_fuzz/src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/hex -runs=1000000`
gives 681 seconds with those three lines and 533 seconds without them. Exec/s are reasonable (>1000) for both versions of the harness. The slow down isn't too drastic, so if these added checks are useful in any way then I think it's fine to keep them.
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1743946931)
On my machine, running
`FUZZ=hex build_fuzz/src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/hex -runs=1000000`
gives 681 seconds with those three lines and 533 seconds without them. Exec/s are reasonable (>1000) for both versions of the harness. The slow down isn't too drastic, so if these added checks are useful in any way then I think it's fine to keep them.
💬 hebasto commented on pull request "build: Fix / improve coverage scripts":
(https://github.com/bitcoin/bitcoin/pull/30772#issuecomment-2329314110)
Rebased to resolve a conflict with the merged https://github.com/bitcoin/bitcoin/pull/30804.
(https://github.com/bitcoin/bitcoin/pull/30772#issuecomment-2329314110)
Rebased to resolve a conflict with the merged https://github.com/bitcoin/bitcoin/pull/30804.
💬 maflcko commented on pull request "build: Fix / improve coverage scripts":
(https://github.com/bitcoin/bitcoin/pull/30772#issuecomment-2329353893)
review ACK d9fcbfc3727e06e6f57d4ab09861f3212d558426
(https://github.com/bitcoin/bitcoin/pull/30772#issuecomment-2329353893)
review ACK d9fcbfc3727e06e6f57d4ab09861f3212d558426
⚠️ ryanofsky opened an issue: "cmake: passing options from depends build to main build"
(https://github.com/bitcoin/bitcoin/issues/30813)
_Originally posted by @hebasto in https://github.com/bitcoin/bitcoin/issues/30800#issuecomment-2326899886_
I think we should first prioritize coordinating the optimization and debugging flags between the depends subsystem and the main build system:
1. Should the optimization and debugging flags coincide, or are they allowed to differ?
2. If the optimization and debugging can differ between the depends subsystem and in the main build system, which should take precedence?
(https://github.com/bitcoin/bitcoin/issues/30813)
_Originally posted by @hebasto in https://github.com/bitcoin/bitcoin/issues/30800#issuecomment-2326899886_
I think we should first prioritize coordinating the optimization and debugging flags between the depends subsystem and the main build system:
1. Should the optimization and debugging flags coincide, or are they allowed to differ?
2. If the optimization and debugging can differ between the depends subsystem and in the main build system, which should take precedence?
💬 fanquake commented on issue "cmake: passing options from depends build to main build":
(https://github.com/bitcoin/bitcoin/issues/30813#issuecomment-2329365908)
I think this is a duplicate of #29796?
(https://github.com/bitcoin/bitcoin/issues/30813#issuecomment-2329365908)
I think this is a duplicate of #29796?
💬 ryanofsky commented on issue "cmake: passing options from depends build to main build":
(https://github.com/bitcoin/bitcoin/issues/30813#issuecomment-2329370412)
Created a separate issue for this because I have some thoughts about it and don't want to hijack the discussion in #30800 which is more about _FORTIFY_SOURCE and detecting which optimization flags are set (as opposed to setting them). Working on an idea which I'll post below.
(https://github.com/bitcoin/bitcoin/issues/30813#issuecomment-2329370412)
Created a separate issue for this because I have some thoughts about it and don't want to hijack the discussion in #30800 which is more about _FORTIFY_SOURCE and detecting which optimization flags are set (as opposed to setting them). Working on an idea which I'll post below.
💬 achow101 commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#issuecomment-2329379225)
ACK a3108a7c5692d137b70b8442b4741936277e89be
(https://github.com/bitcoin/bitcoin/pull/30808#issuecomment-2329379225)
ACK a3108a7c5692d137b70b8442b4741936277e89be
📝 TheCharlatan opened a pull request: "kernel: Create usable static kernel library"
(https://github.com/bitcoin/bitcoin/pull/30814)
Since the move to cmake, the kernel static library that is installed after a cmake --install build is unusable. It lacks symbols for the internal libraries, besides those defined in the kernel library target.
Fix this by explicitly adding the required object files of its dependencies to the library instead of relying on additional linker
steps.
A generator expression guard is added to ensure that libraries that are not compatible with the target architecture are not compiled. It is not cl
...
(https://github.com/bitcoin/bitcoin/pull/30814)
Since the move to cmake, the kernel static library that is installed after a cmake --install build is unusable. It lacks symbols for the internal libraries, besides those defined in the kernel library target.
Fix this by explicitly adding the required object files of its dependencies to the library instead of relying on additional linker
steps.
A generator expression guard is added to ensure that libraries that are not compatible with the target architecture are not compiled. It is not cl
...
💬 maflcko commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-2329386280)
re-ACK 5b59cfc944f9eac73e1e69bcc660280b6809724a 🌞
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 5b59cfc944f9eac73e1e
...
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-2329386280)
re-ACK 5b59cfc944f9eac73e1e69bcc660280b6809724a 🌞
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 5b59cfc944f9eac73e1e
...