💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1817848501)
Should keep using `TrimStringView` here to avoid allocating extra `std::string` instances. Fuzzing code is re-run at high frequency so optimization (without compromising code coverage) is important.
```suggestion
assert(encoded_string == TrimStringView(random_string));
```

💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1817851827)
Why add the space?

Could also verify error is returned correctly.
```suggestion
assert(DecodeBase64PSBT(psbt, random_string, error) == error.empty());
```

💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1817849205)
I understand the change to use `random_string` as something that might either be encoded already, or as something to be encoded in the later half of these functions, so the rename makes sense. Would avoid stripping `const` for these slightly longer lived variables, just for peace of mind.
```suggestion
const std::string random_string{buffer.begin(), buffer.end()};
const std::vector<unsigned char> random_data{ToByteVector(random_string)};
```
---
Another aspect is that `buffer` has
...

💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1817857203)
(My instinct would be to go with the following to avoid rocking the boat when it comes to seed corpus naming etc, but I'm too new to this aspect of fuzzing to carry much weight in this aspect).
```C++
void base58_encode_decode(FuzzBufferType buffer);
void base58check_encode_decode(FuzzBufferType buffer);
void base32_encode_decode(FuzzBufferType buffer);
void base64_encode_decode(FuzzBufferType buffer);
void psbt_base64_decode(FuzzBufferType buffer);

FUZZ_TARGET(base_encode_decode)
{

...

💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1817854496)
Good that you removed the normalizing of the case for Base58 roundtripping as case is significant there (unlike how we treat base32). :+1:

💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1817861317)
Could test slightly more aggressively in line with the removed test:
https://github.com/bitcoin/bitcoin/blob/a5b4e42a575b4680a9eedb6ab2275d2a723cc20d/src/test/base58_tests.cpp#L93-L94

```suggestion
assert(encoded_string.empty() || !DecodeBase58(encoded_string, decoded, fuzzed_data_provider.ConsumeIntegralInRange(0, decoded.size() - 1)));
```

💬 edilmedeiros commented on issue "Creating too many wallets exhausts file descriptor limit and leads to crash":
(https://github.com/bitcoin/bitcoin/issues/27732#issuecomment-2439607898)
I confirm this is still happening on v28.

🤔 glozow reviewed a pull request: "rpc: getorphantxs follow-up"
(https://github.com/bitcoin/bitcoin/pull/31043#pullrequestreview-2397270441)
utACK 0ea84bc362f395fd247623c22942eb5ca3d1b874

💬 jasonribble commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1817901340)
maybe you can rename the "18" value as a BnB threshold

💬 davidgumberg commented on pull request "util: Remove RandAddSeedPerfmon":
(https://github.com/bitcoin/bitcoin/pull/31124#issuecomment-2439688459)
post-merge utACK https://github.com/bitcoin/bitcoin/pull/31124/commits/9bb92c0e7ffe2426b4afb80ddd4b4c96968316b5

💬 davidgumberg commented on pull request "Windows bitcoind stall debugging [NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2439690252)
> Turned off hourly CI for now, since fix removing the offending function was merged into bitcoin:master.

Would it make sense to leave it running for a little longer to validate that the fix worked?

💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2439723285)
Code review ACK e01bb3a06693efc77a173afdf25ed7ba631316a2

💬 hodlinator commented on pull request "Windows bitcoind stall debugging [NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2439723709)
> Would it make sense to leave it running for a little longer to validate that the fix worked?

My reasoning was that there should be enough CI runs on master and all runs on PRs based on top of the merged fix. If it re-occurs hopefully one of us will reopen #30390. But I don't have a well-developed sense for how much strain is reasonable to put on the CI resources.

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1817922747)
Reorgs are split: additions happen via the changesets (since we just use ATMP), while removals happen in their own special funky way.

I can update the comment.

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1817923233)
Additions never happen outside a changeset. RBF-related removals always go through a changeset as well.

Removals that happen because a block is found, or due to a reorg, or due to mempool expiry or mempool limiting do not go through the changeset.

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1817924314)
Not sure I follow -- if somehow the PolicyScriptChecks() passed for something that failed ConsensusChecks(), then this logic would trigger and we would then remove the transactions from the mempool. It's slightly different than the old behavior of never allowing it to enter in the first place, but given that we're holding the mempool locks the whole time it is not really directly observable?

💬 theStack commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2439735659)
Concept ACK

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1817930330)
Fixed, thanks.

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1817930359)
Fixed, thanks.

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1817930369)
Fixed, thanks.