💬 yancyribbens commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2725992821)
> Which commit is this that you are linking to?

This is master from only a few days ago when I submitted the review feedback.

👍 darosior approved a pull request: "versionbits refactoring"
(https://github.com/bitcoin/bitcoin/pull/29039#pullrequestreview-2687058377)
ACK e3014017bacff42d8d69f3061ce1ee621aaa450a

💬 yancyribbens commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1996429149)
Good idea to simplify this by reducing the size of the set. Two sevens here with a reduced target amount serves the same purpose as four sevens.

💬 yancyribbens commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1996431719)
This is really minor nit, although I find the `/*selection_target=*` comments to be more distracting than helpful personally.

💬 yancyribbens commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1996437543)
It would be nice if BnB returned the `total_tries`, like coin_grinder. I recently added that to the rust implementation since I think it's useful for regression testing. If BnB was ever refactored or had changes it would come in handy imo.

💬 yancyribbens commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2726030060)
> We would expect at low feerates the input set with more inputs to be preferred, but instead the selection with the smaller change budget is preferred.

I'm familiar with change_budget as it exists in coin-grinder. Beyond that, I'm not sure how that ties in to BnB and SRD.

> Please see the commit "test: Move BnB feerate sensitivity tests" and the commit message of the commit.

I reviewed the commits, and it _appears_ that the test coverage is equivalent. It might be helpful to document
...

💬 pinheadmz commented on pull request "[draft] Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-2726033041)
> My understanding from the low-level networking discussion at CoreDev was that this wouldn't build on top of sockman. I guess the devil is in the details but can you address that in what sense the current approach follows what was discussed there? Thanks!

Sure, by coredev I had already written most of this implementation (based on sockman) but the performance was bad, and that was part of the motivation behind the deep-dive talk. However, by the end of the week I had reviewed that code in pe
...

🤔 Prabhat1308 reviewed a pull request: "qa: Fix TxIndex race conditions"
(https://github.com/bitcoin/bitcoin/pull/32010#pullrequestreview-2687270174)
Concept ACK [`3301d2c`](https://github.com/bitcoin/bitcoin/pull/32010/commits/3301d2cbe8c3b76c97285d75fa59637cb6952d0b)
I could not recreate a race condition but I have
- Tested that the commented files dont trigger a startup error.
- Ran all the tests to check that all pass .

💬 1440000bytes commented on pull request "i2p: make a time gap between creating transient sessions and using them":
(https://github.com/bitcoin/bitcoin/pull/32065#issuecomment-2726286747)
Concept ACK

💬 ajtowns commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1996622142)
> I thought the only reason you had this logic here was to convey to the RPC whether to show these fields, which is a bit convoluted. I don't think this is worth touching at this point.

Ah, no, it's two separate things in my head: the versionbits code quoted above is making sure the values returned in the structure are as sane as they can be, and the RPC code is taking a special case that wouldn't make sense (threshold=0 always implies possible=true if the signalling means anything) and tryin
...

📝 maflcko opened a pull request: "contrib: Make deterministic-coverage error messages more readable"
(https://github.com/bitcoin/bitcoin/pull/32074)
This is almost a "refactor" to tidy up the error messages. Apart from the messages, the behavior of the tools is identical.

This was requested in https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1969502508.

💬 maflcko commented on pull request "test: testnet4 could log a disk space warning":
(https://github.com/bitcoin/bitcoin/pull/32057#discussion_r1996663663)
not sure about logging a warning, then checking it and manually removing it, when the warning isn't even part of the test. It seems easier to just avoid the warning by setting `prune=whatever`?

I expect this will make the testnet3 removal diff a tiny bit smaller, as well as the testnet5 addition diff.

🤔 maflcko reviewed a pull request: "build: Drop option to disable hardening."
(https://github.com/bitcoin/bitcoin/pull/32071#pullrequestreview-2687522118)
review ACK ecf2046d4b5c43ddf64f62f09cd3ed70dd5caafb 📮

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK ecf2046d4b5c
...

💬 maflcko commented on pull request "test: Update coverage.cpp to drop linux restriction":
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2726365221)
review-only ACK ecdbe72916c1cabbf810e892d25fe6eed30b1306

💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1996678675)
:nail_care: Applied nail polish to the messages in https://github.com/bitcoin/bitcoin/pull/32074

🤔 janb84 reviewed a pull request: "test: Use rpc_deprecated only for testing deprecation"
(https://github.com/bitcoin/bitcoin/pull/31977#pullrequestreview-2687613840)
re ACK [2819c51](https://github.com/bitcoin/bitcoin/commit/2819c514825577dc8d73690882f25117d412ed9f)
Thank you for rewriting the comment - the intention is much easier to understand now (IMO)

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1996761714)
It shouldn't be, but annoyingly this is not trivial to change. Since the functions constructing the params in our code only return const types, we have to carry that const into our API. I think the only alternative is copying the params, which I will push shortly.

🤔 polespinasa reviewed a pull request: "test: Use rpc_deprecated only for testing deprecation"
(https://github.com/bitcoin/bitcoin/pull/31977#pullrequestreview-2687625876)
reACK 2819c514825577dc8d73690882f25117d412ed9f

⚠️ farrelA opened an issue: "SAI-15 and i can execution idea."
(https://github.com/bitcoin/bitcoin/issues/32075)
### Please describe the feature you'd like to see added.

// SAI-15 Key Generation and Encryption Framework
def generate_key(self):
return secrets.token_bytes(399)

def encrypt(self, plaintext, key):
if len(key) != 399:
raise ValueError("Key must be exactly 399 bytes")
ciphertext = bytearray()
for i, byte in enumerate(plaintext):
key_byte = key[i % 399]
ciphertext.append(byte ^ key_byte)
return bytes(ciphertext)

// Message Schedule and Compression for
...

✅ maflcko closed an issue: "SAI-15 and i can execution idea."
(https://github.com/bitcoin/bitcoin/issues/32075)