💬 fjahr commented on pull request "index: Fix coinstats overflow and introduce index versioning":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2107034420)
> This all looks good to me, but I am not sure if embedding the database in our functional test suite is a good idea. I don't think it is too much data, but I am not sure how stable it is in terms of file formats. Is this really compatible with all posix systems? Then again it could just be removed once/if it starts becoming unreliable.

> It is "just" 20kB, but I think we'd want to avoid adding binary blobs to the repo at this point. At a minimum, it would be good to have a reason to do this.
...

💬 furszy commented on pull request "wallet: re-activate "AmountWithFeeExceedsBalance" error":
(https://github.com/bitcoin/bitcoin/pull/25269#issuecomment-2909243696)
Updated per feedback. Thanks murchandamus!

💬 furszy commented on pull request "wallet: re-activate "AmountWithFeeExceedsBalance" error":
(https://github.com/bitcoin/bitcoin/pull/25269#discussion_r2107038658)
good catch!, fixed. This also revealed that the optional field `CoinsResult::total_effective_amount` has never being optional (it was being set to 0 during construction).

💬 furszy commented on pull request "wallet: re-activate "AmountWithFeeExceedsBalance" error":
(https://github.com/bitcoin/bitcoin/pull/25269#discussion_r2107039968)
sure, added.

💬 maflcko commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#issuecomment-2909269900)
Regtest is only used for testing and heavily mockable, so one should be cautious to put too much meaning into a value that even on mainnet is informational-only at best. I am happy to drop commit faf6304bdfdf228354b4072b72f4c0ef90fdaade, if the test is too confusing. Also note, generally it is best to start a code-related discussion thread in the related piece of code inline. This ensures that discussion is bundled nicely in one thread, and can be resolved/skimmed/reopened independent of the mai
...

💬 maflcko commented on pull request "index: Fix coinstats overflow and introduce index versioning":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2107076197)
> Or maybe you would discard the corruption test and only do the happy path test?

Yeah, I wonder if we can reliably test anything other than the happy path anyway. Generally, once UB happened, there is no reliable way to recover from it. While in practise here, most likely you'll end up with negative values, I don't think this is guaranteed by the C++ standard and probably not something to be able to rely on.

Taking a step back, the size of the index is probably small and it is not too har
...

💬 fjahr commented on pull request "threading: remove ancient CRITICAL_SECTION macros":
(https://github.com/bitcoin/bitcoin/pull/32592#issuecomment-2909347965)
Concept ACK

🤔 BrandonOdiwuor reviewed a pull request: "test: fix and augment block tests of invalid_txs"
(https://github.com/bitcoin/bitcoin/pull/32591#pullrequestreview-2868092029)
Code Review ACK 8fcd6845052354fad80ae7e5feda3f6a2e441e12

💬 polespinasa commented on pull request "rpc: generateblock to allow multiple outputs":
(https://github.com/bitcoin/bitcoin/pull/32468#discussion_r2107133216)
done!

💬 polespinasa commented on pull request "rpc: generateblock to allow multiple outputs":
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2909407379)
- OP_RETURN fallback if no address or descriptor set is provided.
- A few tests added to test new functionalities

💬 polespinasa commented on pull request "rpc: generateblock to allow multiple outputs":
(https://github.com/bitcoin/bitcoin/pull/32468#discussion_r2107136482)
I've set "output" as optional but still I have to provide at least an empty list. I'm unable to see how to make it fully optional.
```
$ ./bitcoin-cli -rpcport=18443 generateblock
error code: -3
error message:
JSON value of type null is not of expected type array
```

```
$ ./bitcoin-cli -rpcport=18443 generateblock []
{
"hash": "25987bbfbce2be6c61393ed95340729ff2cc25c2b26db422b6e43c8a8f4558a0"
}
```

💬 1440000bytes commented on pull request "[Draft/POC] Add secp256k1-based HPKE (Hybrid Public Key Encryption) For Payjoin v2":
(https://github.com/bitcoin/bitcoin/pull/32617#issuecomment-2909433769)
If [`rust-payjoin`](https://github.com/payjoin/rust-payjoin/tree/master/payjoin-cli) already works with bitcoin core wallet, why do we need to add anything in bitcoin core for payjoin?

💬 dergoegge commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-2909514124)
Concept ACK

Thanks for picking this up!

💬 maflcko commented on pull request "rpc: generateblock to allow multiple outputs":
(https://github.com/bitcoin/bitcoin/pull/32468#discussion_r2107200827)
`get_array` is a check function to ensure the type is array. (The type null is not the type array, so the check fails)

The fix would be to not call `get_array` when the type is null.

💬 polespinasa commented on pull request "rpc: generateblock to allow multiple outputs":
(https://github.com/bitcoin/bitcoin/pull/32468#discussion_r2107340130)
oh you're right, solved :)

🤔 janb84 reviewed a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2868443828)
ACK 0bc6ed61cfab6d97e74103efd41c46faf5941ff6

Ratelimits loging to disk if loggin exceeds 1 MiB in 1 hour (WINDOW_MAX_BYTES const & WINDOW_SIZE const)

- code review
- build & tested

💬 janb84 commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2107350361)
nit: would be nice to align the other initializations to the new C++ 11 {} style in a follow up PR, to keep the code base consistent.

🤔 furszy reviewed a pull request: "Including exception what() in Runaway dialog box"
(https://github.com/bitcoin-core/gui/pull/876#pullrequestreview-2868183402)
> If an exception happens, including e.what() in the text passed to the dialog
box, so that the user can see it. This is in addition to any text from
getWarnings() that might already be present, separated by newlines if so.

Have you verified that we won't output the same error message twice?

💬 furszy commented on pull request "Including exception what() in Runaway dialog box":
(https://github.com/bitcoin-core/gui/pull/876#discussion_r2107175788)
const std::string&

💬 furszy commented on pull request "Including exception what() in Runaway dialog box":
(https://github.com/bitcoin-core/gui/pull/876#discussion_r2107175290)
`const std::string&`