💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2217568262)
Done

💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2217568307)
I've dropped this commit from this branch.

💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2217568482)
Done

💬 optout21 commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2217569237)
Very low relevance, but "updating the fee logic" is a nit confusing to me:
- why "updating" and not "update"? "remove ..., updating ..." suggests it's a side effect in the same step as opposed to a separate step
- is it `(updating the fee) logic` or `updating the (fee logic)`? What logic is updated?

My suggestion: `also update fee stats` or just `also update fees`.
Otherwise LGTM.

💬 ajtowns commented on pull request "tests: speed up coins_tests by parallelizing":
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3093204312)
> This branch (with a small patch):

Nice, that seems to have fixed my cmake errors.

💬 l0rinc commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2217569835)
"Remove... while also update" -> "Remove..., updating".

If you agree with the change please comment an uppercase ACK with the latest commit hash to your message.

💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2217570563)
Maybe they deserve their own PR?

💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2217570887)
Don't we need to derive the scan and spend keys?

💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2217571215)
In the `OutputType::SILENT_PAYMENTS` case, I set the `desc_str` not the `desc_prefix`; I have to modify the `assert` to prevent a failure.

💬 optout21 commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#issuecomment-3093209323)
ACK 249889bee6b88eb9814eb969e6fb108f86a4bf98

💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2217572750)
The derived spend private key must be saved to the DB by the wallet.

We can avoid this by doing what other descriptors do; the sp descriptor will then be in this form `sp(xpriv/352h/0h/0h/1h/0,xpub/352h/0h/0h/0h/0)` . The `Parse` function will then derive the scan key and save it in the descriptor. The spend key can be derived later when needed from the master key.

With this alternative method, the wallet only saves the master key to DB as it has always done.

💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2217573106)
We only use the reference once, so it's not needed. I'll take this out as I retouch.

💬 ajtowns commented on pull request "script: return verification flag responsible for error upon validation failure":
(https://github.com/bitcoin/bitcoin/pull/33012#issuecomment-3093224008)
> Note this is a slight behaviour change, as a consensus-related Script validation failure that happens after a standardness-related Script validation failure would not be treated as a consensus error anymore (and consequentially the peer not disconnected).

I'm skeptical whether this behaviour is really worth preserving in a limited fashion? With this change, an attacker can waste your resources without being discouraged or risking having to pay tx fees by making a consensus invalid tx that f
...

💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2217694773)
Updated

💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2217694788)
Updated

💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2217694846)
I broke this into 3 commits.

💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2217695331)
I ended up leaving the reference and used it in 2 other locations. The code looks slightly cleaner this way.

💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#issuecomment-3094352453)
Added @achow101 as co-author on commits with code/ideas taken from https://github.com/bitcoin/bitcoin/pull/28453

💬 hebasto commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2217735632)
b113877545a1c83b470a380402b4409aa02c8282

On Alpine Linux v3.22, using GCC 14.2.0:
```
[ 74%] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/txorphan.cpp.o
In file included from /bitcoin/src/script/script.h:10,
from /bitcoin/src/primitives/transaction.h:11,
from /bitcoin/src/consensus/validation.h:11,
from /bitcoin/src/test/fuzz/txorphan.cpp:6:
/bitcoin/src/crypto/common.h: In function 'void txorphanage_sim_fuzz_target(FuzzBuffer
...

🤔 OrangeDoro reviewed a pull request: "test: revive test verifying that `GetCoinsCacheSizeState` switches from OK→LARGE→CRITICAL"
(https://github.com/bitcoin/bitcoin/pull/33021#pullrequestreview-3035969517)
Hi! I'm a grad student working on a research project about using large language models to automate code review. Based on your commit 966bbabbd69039a2c7a03429c783f7d6e6a7c2a7 and the changes in src/test/validation_flush_tests.cpp, my tool generated this comment:
1. **Dynamic Memory Usage Check**: Ensure that the expected behavior of `DynamicMemoryUsage()` aligns with the assumptions made in this test.
2. **Dynamic Memory Usage Checks**: The checks for `view.DynamicMemoryUsage()` are essential t
...