🤔 stickies-v reviewed a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2125109551)
Approach ACK 63923c8da686da42f522771f338ea8f2a4f4e568

💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1644210320)
nit: declaring `requested_num_elems` as a `uint32_t` would seem more intuitive:

<details>
<summary>git diff on 8960bc986b</summary>

```diff
diff --git a/src/cuckoocache.h b/src/cuckoocache.h
index c9bd45befc..85556a4da5 100644
--- a/src/cuckoocache.h
+++ b/src/cuckoocache.h
@@ -364,10 +364,10 @@ public:
*/
std::pair<uint32_t, size_t> setup_bytes(size_t bytes)
{
- size_t requested_num_elems = bytes / sizeof(Element);
- if (std::numeric_limits<uint32_
...

💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1645009465)
If I understand correctly, `m_script_execution_cache_hasher` is a pre-initialized hasher that we want to copy every time we use it, and we definitely don't want to modify in-place. I think having it as a public struct member is not very robust to enforce that, perhaps better to use a setter here? E.g. something like:

<details>
<summary>git diff on 63923c8da6</summary>

```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 39ab7b56ab..600c427c93 100644
--- a/src/validation.c
...

💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1644415772)
To minimize future footguns by accidentally passing a cache by value, perhaps best to disable the copy constructor and copy assignment operator? (+ same for `CSignatureCache` in 63923c8da686da42f522771f338ea8f2a4f4e568)

<details>
<summary>git diff on 1409fa4f41</summary>

```diff
diff --git a/src/validation.h b/src/validation.h
index 9f858bff1d..90f484bc9f 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -369,6 +369,10 @@ static_assert(std::is_nothrow_destructible_v<CScriptChec
...

💬 hebasto commented on pull request "[DO NOT MERGE] Erlay: bandwidth-efficient transaction relay protocol (Full implementation)":
(https://github.com/bitcoin/bitcoin/pull/30277#issuecomment-2176997916)
@sr-gi

Feel free to grab https://github.com/hebasto/bitcoin/commit/ffed2ab27465e4e3888b4139ca50db1226a66ec5 to fix the MSVC build.

💬 AngusP commented on pull request "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`":
(https://github.com/bitcoin/bitcoin/pull/30237#discussion_r1645152984)
Addressed in 8df9d16

💬 AngusP commented on pull request "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`":
(https://github.com/bitcoin/bitcoin/pull/30237#discussion_r1645153389)
Addressed in 8df9d16

🤔 furszy reviewed a pull request: "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB"
(https://github.com/bitcoin/bitcoin/pull/26596#pullrequestreview-2126669884)
Just a small comment; you don't have to take it.

I believe this PR could get merged quite fast if you leave 89503710386f52d9162f67fcd707eceffa954faa for a follow-up and make `IsMine` compatible with `LegacyDataSPKM` in this PR.

I'm suggesting this because this PR starts testing the newly introduced BDB reader class and allows people to use it, which is great since we want it polished for the next release. The `IsMine` removal is also nice but I wouldn't miss a deadline for it.

💬 1440000bytes commented on pull request "chainparams: Add achow101 DNS seeder":
(https://github.com/bitcoin/bitcoin/pull/30007#issuecomment-2177227865)
Recently 27.1 was released and nothing was include din DNS seed.. Sad and Cant do anything,

It is less likely they will secure domain in a domain takeover attack.

I do not trust them and adding one more including improves security.

💬 tdb3 commented on pull request "test: Validate oversized transactions or without inputs":
(https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1645219537)
Nice approach to test the boundary and handle non-`bad-txns-oversize` reject reasons.

Exercised this by inducing a failure with:
```diff
- auto maxPayloadSize = maxTransactionSize - oversizedTransactionBaseSize;
+ auto maxPayloadSize = maxTransactionSize - oversizedTransactionBaseSize +1;
```
The test failed as expected.

👍 tdb3 approved a pull request: "test: Validate oversized transactions or without inputs"
(https://github.com/bitcoin/bitcoin/pull/29862#pullrequestreview-2126734529)
ACK 969e047cfbab86e5819a2c9056e8d2dab17513a8
Left comments showing what was exercised.

💬 tdb3 commented on pull request "test: Validate oversized transactions or without inputs":
(https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1645225922)
Similarly, exercised this by inducing a failure with:
```diff
- maxPayloadSize += 1;
+ //maxPayloadSize += 1;
```

The test failed as expected.

💬 theStack commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2177289493)
> It's about 25 GB. Not sure if it's supposed to be deterministic, but here you go:
>
> ```
> 99450bac6e081e03751c064034b3d10e17b463ca56dfc35797fc6b5fe0de8ac4 ../utxo-snapshots/utxo-840000.sqlite
> ```

Pretty sure that the .sqlite file as a whole is not deterministic, as there are e.g. different page sizes supported (similar to BDB) and the header seems to include the concrete SQLite version that was used to create the file, see https://www.sqlite.org/fileformat.html#the_database_header
...

⚠️ kosuodhmwa opened an issue: "What means "Mempool limited" ?"
(https://github.com/bitcoin/bitcoin/issues/30303)
https://github.com/Mirobit/bitcoin-node-manager/issues/92

💬 S3RK commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#issuecomment-2177884124)
reACK a9c7300135f0188daa5bce5491e2daf2dd8da8ae

Looks even simpler now, thanks for incorporating feedback.

💬 Sjors commented on pull request "upnp: fix build with miniupnpc 2.2.8":
(https://github.com/bitcoin/bitcoin/pull/30283#issuecomment-2177898915)
Guix hashes

```
2f879df88862b94c211670a451f442eb951a6ec963d841be683d1ee7c14cf49a guix-build-8acdf6654083/output/aarch64-linux-gnu/SHA256SUMS.part
00f163274f35e955eb5ef00cb1e6e81e828b3b7415d15f594ea61bc921c000b9 guix-build-8acdf6654083/output/aarch64-linux-gnu/bitcoin-8acdf6654083-aarch64-linux-gnu-debug.tar.gz
e83b5155ab258164204fd117f7974af90b580c2f0f0da713a1066eb2caecf651 guix-build-8acdf6654083/output/aarch64-linux-gnu/bitcoin-8acdf6654083-aarch64-linux-gnu.tar.gz
21ac9b233e814a1393
...

💬 S3RK commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#discussion_r1645529346)
> It also enforces that the sequences are either MAX_SEQUENCE_NONFINAL or MAX_BIP125_RBF_SEQUENCE, so this needs to be checking that as well otherwise an assertion will be hit. Or a commit could be added to relax those checks.

It seems like this part of the comment wasn't addressed

✅ maflcko closed an issue: "What means "Mempool limited" ?"
(https://github.com/bitcoin/bitcoin/issues/30303)

💬 maflcko commented on issue "What means "Mempool limited" ?":
(https://github.com/bitcoin/bitcoin/issues/30303#issuecomment-2177906842)
Usually the issue tracker is used to track technical issues related to the Bitcoin Core code base.

General bitcoin questions and/or support requests are best directed to the [Bitcoin StackExchange](https://bitcoin.stackexchange.com) or the `#bitcoin` IRC channel on Libera Chat, or one of the Bitcoin subreddits, or any other place that you feel is well suited.

💬 kosuodhmwa commented on issue "What means "Mempool limited" ?":
(https://github.com/bitcoin/bitcoin/issues/30303#issuecomment-2177910080)
ok thx