📝 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
---
...
💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-2107482943)
Rebased on master.
Currently based on #30074 and #30078.
Dropped the commit related to `--ld-path`, and added a commit to remove binutils from the macOS Guix build env.
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-2107482943)
Rebased on master.
Currently based on #30074 and #30078.
Dropped the commit related to `--ld-path`, and added a commit to remove binutils from the macOS Guix build env.
💬 ryanofsky commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1598423939)
re: https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1598392660
> 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.
I think this is only intended to loop if there is an error loading, and the user is using a gui, and they are asked "Do you want to rebuild the block database now?" and they answer yes.
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1598423939)
re: https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1598392660
> 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.
I think this is only intended to loop if there is an error loading, and the user is using a gui, and they are asked "Do you want to rebuild the block database now?" and they answer yes.
💬 cbergqvist commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1598426841)
`ToIntegral` itself returns a `std::optional` unfortunately so the suggested change won't compile as is Super-weird that this error popped up now.
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1598426841)
`ToIntegral` itself returns a `std::optional` unfortunately so the suggested change won't compile as is Super-weird that this error popped up now.
💬 remyers commented on pull request "wallet: add coin selection parameter `add_excess_to_recipient_position` for changeless txs with excess that would be added to fees":
(https://github.com/bitcoin/bitcoin/pull/30080#issuecomment-2107502094)
Thanks for the feedback! I agree that it is worth considering how to make these parameters more useful for general users.
> `change_target` IIUC you want a specific min value for the change UTXO. Would a following alternative work? You provide your change address in the list of recipients with desired min amount and specify it as change UTXO using `change_position`parameter. The downside compared to a new option is that we always have a change output. Not sure if that's a problem in your case
...
(https://github.com/bitcoin/bitcoin/pull/30080#issuecomment-2107502094)
Thanks for the feedback! I agree that it is worth considering how to make these parameters more useful for general users.
> `change_target` IIUC you want a specific min value for the change UTXO. Would a following alternative work? You provide your change address in the list of recipients with desired min amount and specify it as change UTXO using `change_position`parameter. The downside compared to a new option is that we always have a change output. Not sure if that's a problem in your case
...
💬 glozow commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1598432185)
Thanks @ryanofsky! That would explain why I didn't see it loop.
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1598432185)
Thanks @ryanofsky! That would explain why I didn't see it loop.
💬 Sjors commented on pull request "Intro: Have user choose assumevalid":
(https://github.com/bitcoin-core/gui/pull/149#issuecomment-2107510950)
Realistically anyone who wants to use this probably won't do so the first time they install the node. So they'll have to carefully delete the blocks and chainstate from their data dir, wipe the GUI settings and trigger this dialog again.
It seems both easier and safer to encourage such a user to add `reindex-chainstate=1` and `assumevalid=0` to their config file instead.
I don't think we should be cluttering the Intro screen for this, especially when looking ahead to Assume UTXO.
(https://github.com/bitcoin-core/gui/pull/149#issuecomment-2107510950)
Realistically anyone who wants to use this probably won't do so the first time they install the node. So they'll have to carefully delete the blocks and chainstate from their data dir, wipe the GUI settings and trigger this dialog again.
It seems both easier and safer to encourage such a user to add `reindex-chainstate=1` and `assumevalid=0` to their config file instead.
I don't think we should be cluttering the Intro screen for this, especially when looking ahead to Assume UTXO.
💬 maflcko commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1598437856)
I placed the `)` in the wrong place, I think. However, `value_or` exists and should be compile-able. See https://en.cppreference.com/w/cpp/utility/optional/value_or
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1598437856)
I placed the `)` in the wrong place, I think. However, `value_or` exists and should be compile-able. See https://en.cppreference.com/w/cpp/utility/optional/value_or
💬 josibake commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2107527748)
Concept ACK
Seems like a reasonable approach to the problem described in #29435 , will dig in more. Using a variant as a replacement for the Enum seems a bit odd at first glance and requires a lot of duplicate code for each struct. Perhaps another approach could be a class for `RemovalReason`, which contains the Enum as a field and then uses a std::variant to hold the data needed by the wallet, if any. Would be more extensible, and then the to string method could be defined once on the class
...
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2107527748)
Concept ACK
Seems like a reasonable approach to the problem described in #29435 , will dig in more. Using a variant as a replacement for the Enum seems a bit odd at first glance and requires a lot of duplicate code for each struct. Perhaps another approach could be a class for `RemovalReason`, which contains the Enum as a field and then uses a std::variant to hold the data needed by the wallet, if any. Would be more extensible, and then the to string method could be defined once on the class
...
💬 maflcko commented on issue "Load PSBT error: Unable to decode PSBT":
(https://github.com/bitcoin/bitcoin/issues/30070#issuecomment-2107530835)
Are you able to reproduce in regtest, with exact steps to reproduce?
(https://github.com/bitcoin/bitcoin/issues/30070#issuecomment-2107530835)
Are you able to reproduce in regtest, with exact steps to reproduce?
💬 TheCharlatan commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1598447607)
I'm not too concerned over the message being printed again, since there a whole bunch of log lines that get printed again too. The for loop should also only be executed once more, since if there is a failure on its second pass, the `reindex` option is true, making it return an `InitError`.
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1598447607)
I'm not too concerned over the message being printed again, since there a whole bunch of log lines that get printed again too. The for loop should also only be executed once more, since if there is a failure on its second pass, the `reindex` option is true, making it return an `InitError`.
💬 glozow commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1598450015)
Thanks sgtm :+1: I didn't want to ignore the comment, but also wasn't sure how to test.
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1598450015)
Thanks sgtm :+1: I didn't want to ignore the comment, but also wasn't sure how to test.
💬 maflcko commented on pull request "tests: improve wallet multisig descriptor test and docs":
(https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1598465742)
> Done
Did you push the changes?
(https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1598465742)
> Done
Did you push the changes?