💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1817860901)
yeah that's confusing, reworded

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1817860919)
correct, updated comment

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1817860922)
done

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1817860928)
out of date logging, updated

💬 jadijadi commented on issue "か":
(https://github.com/bitcoin/bitcoin/issues/29609#issuecomment-2439602668)
Is this spam / scam? I believe this can be closed.

✅ fanquake closed an issue: "か"
(https://github.com/bitcoin/bitcoin/issues/29609)

:lock: fanquake locked an issue: "か"
(https://github.com/bitcoin/bitcoin/issues/29609)

🤔 hodlinator reviewed a pull request: "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests"
(https://github.com/bitcoin/bitcoin/pull/30746#pullrequestreview-2397249971)
Concept ACK ad917fb105b68fb369455ec7c42292a7673db1ad

- Extra tests/coverage, does not modify files containing the functionality being tested.
- Does remove `base58_random_encode_decode` unit test, but everything seems to be carried over to the modified fuzz test (except for testing for a variable count of insufficient result buffer sizes).

💬 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