✅ maflcko closed an issue: "Bitcoin Core 27 crash at sync - Ubuntu - No error in logs"
(https://github.com/bitcoin/bitcoin/issues/30001)
(https://github.com/bitcoin/bitcoin/issues/30001)
💬 maflcko commented on issue "Bitcoin Core 27 crash at sync - Ubuntu - No error in logs":
(https://github.com/bitcoin/bitcoin/issues/30001#issuecomment-2107260441)
Closing for now due to inactivity, but please leave a comment if you have more details or information.
(https://github.com/bitcoin/bitcoin/issues/30001#issuecomment-2107260441)
Closing for now due to inactivity, but please leave a comment if you have more details or information.
💬 maflcko commented on pull request "kernel: Remove batchpriority from kernel library":
(https://github.com/bitcoin/bitcoin/pull/30083#issuecomment-2107333523)
utACK 04ffe4061da2d0135f206032e167703772b5da78 🎪
<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: utACK 04ffe4061da2d0135f2060
...
(https://github.com/bitcoin/bitcoin/pull/30083#issuecomment-2107333523)
utACK 04ffe4061da2d0135f206032e167703772b5da78 🎪
<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: utACK 04ffe4061da2d0135f2060
...
💬 fanquake commented on pull request "contrib: use ENV flags in get_arch":
(https://github.com/bitcoin/bitcoin/pull/30074#issuecomment-2107335721)
Guix Build (aarch64):
```bash
fcb75f2a3b61befeabe143301e5db490d23b1ee4a5edb109f219dea575395b49 guix-build-b59a027d957a/output/aarch64-linux-gnu/SHA256SUMS.part
15310e3aff2a9ef71bad06315fcd425e56611d5bed7f6c394f3fe2248140e17b guix-build-b59a027d957a/output/aarch64-linux-gnu/bitcoin-b59a027d957a-aarch64-linux-gnu-debug.tar.gz
8e60146d39a47e9c332e3b842e239f1ca7773ca2e6788876155377f4568ab1fa guix-build-b59a027d957a/output/aarch64-linux-gnu/bitcoin-b59a027d957a-aarch64-linux-gnu.tar.gz
104322
...
(https://github.com/bitcoin/bitcoin/pull/30074#issuecomment-2107335721)
Guix Build (aarch64):
```bash
fcb75f2a3b61befeabe143301e5db490d23b1ee4a5edb109f219dea575395b49 guix-build-b59a027d957a/output/aarch64-linux-gnu/SHA256SUMS.part
15310e3aff2a9ef71bad06315fcd425e56611d5bed7f6c394f3fe2248140e17b guix-build-b59a027d957a/output/aarch64-linux-gnu/bitcoin-b59a027d957a-aarch64-linux-gnu-debug.tar.gz
8e60146d39a47e9c332e3b842e239f1ca7773ca2e6788876155377f4568ab1fa guix-build-b59a027d957a/output/aarch64-linux-gnu/bitcoin-b59a027d957a-aarch64-linux-gnu.tar.gz
104322
...
💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1598332839)
Oops, this is a bad copypaste. I forgot to delete this commen I copied over.
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1598332839)
Oops, this is a bad copypaste. I forgot to delete this commen I copied over.
💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1598333278)
Same - forgot to delete, sorry
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1598333278)
Same - forgot to delete, sorry
💬 maflcko commented on issue "Slow memory leak in v22.0?":
(https://github.com/bitcoin/bitcoin/issues/24542#issuecomment-2107357612)
> And if I understand correctly, it is then copied _again_ [here](https://github.com/bitcoin/bitcoin/blob/98dd4e712efaa2b77cb168426756879c6405c3f4/src/univalue/lib/univalue.cpp#L134) since `UniValue` doesn't have a move constructor. So improving UniValue move semantics seems to make sense here indeed.
Can you explain this? It has a move constructor, at least the following compiles for me:
```diff
diff --git a/src/univalue/lib/univalue.cpp b/src/univalue/lib/univalue.cpp
index 656d2e8203.
...
(https://github.com/bitcoin/bitcoin/issues/24542#issuecomment-2107357612)
> And if I understand correctly, it is then copied _again_ [here](https://github.com/bitcoin/bitcoin/blob/98dd4e712efaa2b77cb168426756879c6405c3f4/src/univalue/lib/univalue.cpp#L134) since `UniValue` doesn't have a move constructor. So improving UniValue move semantics seems to make sense here indeed.
Can you explain this? It has a move constructor, at least the following compiles for me:
```diff
diff --git a/src/univalue/lib/univalue.cpp b/src/univalue/lib/univalue.cpp
index 656d2e8203.
...
💬 fanquake commented on pull request "depends: set AR & RANLIB for CMake":
(https://github.com/bitcoin/bitcoin/pull/30078#discussion_r1598351939)
> What works for me is:
> Bug here: https://gitlab.kitware.com/cmake/cmake/-/issues/18087
Given the bug report is 5 years old, I can't imagine CMake is going to either "fix" or change things now. I can switch to `CMAKE_` using `which` if that is what's preferred. Will add `CMAKE_NM` even if that's missing from the CMake docs. Maybe we can upstream a fix for that as well. Whatever will move things forward.
(https://github.com/bitcoin/bitcoin/pull/30078#discussion_r1598351939)
> What works for me is:
> Bug here: https://gitlab.kitware.com/cmake/cmake/-/issues/18087
Given the bug report is 5 years old, I can't imagine CMake is going to either "fix" or change things now. I can switch to `CMAKE_` using `which` if that is what's preferred. Will add `CMAKE_NM` even if that's missing from the CMake docs. Maybe we can upstream a fix for that as well. Whatever will move things forward.
👍 brunoerg approved a pull request: "fuzz: wallet, add target for `Crypter`"
(https://github.com/bitcoin/bitcoin/pull/28074#pullrequestreview-2052516066)
utACK d7290d662f494503f28e087dd728b492c0bb2c5f
(https://github.com/bitcoin/bitcoin/pull/28074#pullrequestreview-2052516066)
utACK d7290d662f494503f28e087dd728b492c0bb2c5f
📝 fanquake converted_to_draft a pull request: "build: use `-no_exported_symbols` on macOS"
(https://github.com/bitcoin/bitcoin/pull/29072)
Based on #21778.
This reduces the size of the binary by 2-3% when building with `--enable-reduce-exports`.
> -no_exported_symbols
> Useful for main executable that don't have plugins and thus need no symbol exports.
Can be tested with `dyld_info -exports src/bitcoind`. The only exported symbol should be `__mh_execute_header`.
One question is if this should be under `--enable-reduce-exports`, or we just try and use it at all times, similar to `-dead_strip(_dylibs)`.
(https://github.com/bitcoin/bitcoin/pull/29072)
Based on #21778.
This reduces the size of the binary by 2-3% when building with `--enable-reduce-exports`.
> -no_exported_symbols
> Useful for main executable that don't have plugins and thus need no symbol exports.
Can be tested with `dyld_info -exports src/bitcoind`. The only exported symbol should be `__mh_execute_header`.
One question is if this should be under `--enable-reduce-exports`, or we just try and use it at all times, similar to `-dead_strip(_dylibs)`.
💬 fanquake commented on pull request "build: use `-no_exported_symbols` on macOS":
(https://github.com/bitcoin/bitcoin/pull/29072#issuecomment-2107414649)
Rebased, but drafted while based on #21778.
(https://github.com/bitcoin/bitcoin/pull/29072#issuecomment-2107414649)
Rebased, but drafted while based on #21778.
💬 maflcko commented on pull request "rpc: Remove index-based Arg accessor":
(https://github.com/bitcoin/bitcoin/pull/29997#discussion_r1598381548)
Not sure what you mean. This sentence introduces the `Type`, which is referred to in the next sentence, so I don't think it can be removed.
Maybe you can share a diff?
(https://github.com/bitcoin/bitcoin/pull/29997#discussion_r1598381548)
Not sure what you mean. This sentence introduces the `Type`, which is referred to in the next sentence, so I don't think it can be removed.
Maybe you can share a diff?
📝 vasild converted_to_draft a pull request: "util: check for errors after close and read in AutoFile"
(https://github.com/bitcoin/bitcoin/pull/29307)
`fclose(3)` may fail to flush the previously written data to disk, thus a failing `fclose(3)` is as serious as a failing `fwrite(3)`.
Previously the code ignored `fclose(3)` failures. This PR improves the explicit callers to check whether it failed. However there is a design issue that `fclose(3)` is also called from the `AutoFile` destructor. There is no good way to signal a failure to the caller from the destructor. So, if closing from the destructor fails, then log a message (the file name
...
(https://github.com/bitcoin/bitcoin/pull/29307)
`fclose(3)` may fail to flush the previously written data to disk, thus a failing `fclose(3)` is as serious as a failing `fwrite(3)`.
Previously the code ignored `fclose(3)` failures. This PR improves the explicit callers to check whether it failed. However there is a design issue that `fclose(3)` is also called from the `AutoFile` destructor. There is no good way to signal a failure to the caller from the destructor. So, if closing from the destructor fails, then log a message (the file name
...
💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2107449866)
I added the highest output amount for each transaction to the index, which the new `dust` argument for `getsilentpaymentblockdata` can use to filter.
The amount is right shifted by 4 and stored as a single byte. This lets us track a useful range (0 - 4080 sat) with only ~5% overhead.
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2107449866)
I added the highest output amount for each transaction to the index, which the new `dust` argument for `getsilentpaymentblockdata` can use to filter.
The amount is right shifted by 4 and stored as a single byte. This lets us track a useful range (0 - 4080 sat) with only ~5% overhead.
💬 glozow commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1598392660)
> The downside to this patch is that the log line may now be printed more than once in the for loop.
Question: do we expect this to loop many times / under what circumstances? I tried adding logging to see what happens on my node, and I haven't seen it go more than once yet.
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1598392660)
> The downside to this patch is that the log line may now be printed more than once in the for loop.
Question: do we expect this to loop many times / under what circumstances? I tried adding logging to see what happens on my node, and I haven't seen it go more than once yet.
🤔 glozow reviewed a pull request: "[refactor] Check CTxMemPool options in ctor"
(https://github.com/bitcoin/bitcoin/pull/28830#pullrequestreview-2052565010)
ACK d447bdcfb0e38353940e4a7fc89d09482d8d39c3 - code review, light fuzzing
(https://github.com/bitcoin/bitcoin/pull/28830#pullrequestreview-2052565010)
ACK d447bdcfb0e38353940e4a7fc89d09482d8d39c3 - code review, light fuzzing
💬 martinus commented on issue "Slow memory leak in v22.0?":
(https://github.com/bitcoin/bitcoin/issues/24542#issuecomment-2107457736)
>The UniValue [created inside blockToJSON](https://github.com/bitcoin/bitcoin/blob/98dd4e712efaa2b77cb168426756879c6405c3f4/src/rpc/blockchain.cpp#L170) is ~40MB worst-case, but it is then copied into a [second UniValue](https://github.com/bitcoin/bitcoin/blob/98dd4e712efaa2b77cb168426756879c6405c3f4/src/rpc/blockchain.cpp#L196), doubling the required heap size.
I had a PR somewhere where I removed the copy constructor and made it mandatory to call a `.copy()`. Then you'd get a compile error
...
(https://github.com/bitcoin/bitcoin/issues/24542#issuecomment-2107457736)
>The UniValue [created inside blockToJSON](https://github.com/bitcoin/bitcoin/blob/98dd4e712efaa2b77cb168426756879c6405c3f4/src/rpc/blockchain.cpp#L170) is ~40MB worst-case, but it is then copied into a [second UniValue](https://github.com/bitcoin/bitcoin/blob/98dd4e712efaa2b77cb168426756879c6405c3f4/src/rpc/blockchain.cpp#L196), doubling the required heap size.
I had a PR somewhere where I removed the copy constructor and made it mandatory to call a `.copy()`. Then you'd get a compile error
...
💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1598411774)
I think you're right. Might have added this as belt-and-suspenders, but have checked again with non-Guix builds, and I still see the correct lld used, even on systems with other llds installed & similar.
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1598411774)
I think you're right. Might have added this as belt-and-suspenders, but have checked again with non-Guix builds, and I still see the correct lld used, even on systems with other llds installed & similar.
💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1598416494)
> Filtering < 546 dust
I should add that I was using `IsDust()` with `DUST_RELAY_TX_FEE`, which takes SegWit into account to decide the exact dust limit.
> 10% of taproot UTXOs
I looked at the largest output, not all outputs.
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1598416494)
> Filtering < 546 dust
I should add that I was using `IsDust()` with `DUST_RELAY_TX_FEE`, which takes SegWit into account to decide the exact dust limit.
> 10% of taproot UTXOs
I looked at the largest output, not all outputs.
💬 stickies-v commented on pull request "rpc: Remove index-based Arg accessor":
(https://github.com/bitcoin/bitcoin/pull/29997#discussion_r1598417298)
Ah good point, I hadn't considered that the `Type` alias is introduced here. I think a preferable approach to me is to use the actual `typename` instead of aliasing it, such as in this diff. Optionally, I'd also be supportive of renaming `R` to something more descriptive such as `ReturnType`, but even with just `R` I think this is more clear:
<details>
<summary>git diff on fadb3eb57b</summary>
```diff
diff --git a/src/rpc/util.h b/src/rpc/util.h
index 9ba7fcf5e6..8fc598b873 100644
---
...
(https://github.com/bitcoin/bitcoin/pull/29997#discussion_r1598417298)
Ah good point, I hadn't considered that the `Type` alias is introduced here. I think a preferable approach to me is to use the actual `typename` instead of aliasing it, such as in this diff. Optionally, I'd also be supportive of renaming `R` to something more descriptive such as `ReturnType`, but even with just `R` I think this is more clear:
<details>
<summary>git diff on fadb3eb57b</summary>
```diff
diff --git a/src/rpc/util.h b/src/rpc/util.h
index 9ba7fcf5e6..8fc598b873 100644
---
...