💬 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
...
(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
...
(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 .
(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
(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
...
(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.
(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.
(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
...
(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
(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
(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)
(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.
(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
(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
...
(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)
(https://github.com/bitcoin/bitcoin/issues/32075)
💬 maflcko commented on pull request "test: Use rpc_deprecated only for testing deprecation":
(https://github.com/bitcoin/bitcoin/pull/31977#issuecomment-2726465393)
lgtm ACK 2819c514825577dc8d73690882f25117d412ed9f
(https://github.com/bitcoin/bitcoin/pull/31977#issuecomment-2726465393)
lgtm ACK 2819c514825577dc8d73690882f25117d412ed9f
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1996815658)
> This (+ in log_category_to_string() leads to runtime assertion errors for interpreted languages
I think the problem here is that the C enums are weakly typed, and if you use them in weakly typed languages you run into these problems. I think the function signature should already give enough of a hint on what the range of allowed values is. That said, I'll change this to instead return `std::nullopt` ad then return false from there.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1996815658)
> This (+ in log_category_to_string() leads to runtime assertion errors for interpreted languages
I think the problem here is that the C enums are weakly typed, and if you use them in weakly typed languages you run into these problems. I think the function signature should already give enough of a hint on what the range of allowed values is. That said, I'll change this to instead return `std::nullopt` ad then return false from there.
💬 maflcko commented on pull request "fuzz: provide more realistic values to the base58(check) decoders":
(https://github.com/bitcoin/bitcoin/pull/31917#issuecomment-2726501066)
review ACK d5537c18a9034647ba4c9ed4008abd7fee33989e 🚛
<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 d5537c18a903
...
(https://github.com/bitcoin/bitcoin/pull/31917#issuecomment-2726501066)
review ACK d5537c18a9034647ba4c9ed4008abd7fee33989e 🚛
<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 d5537c18a903
...
💬 maflcko commented on pull request "fuzz: provide more realistic values to the base58(check) decoders":
(https://github.com/bitcoin/bitcoin/pull/31917#issuecomment-2726515276)
(In theory this could be backported to 29.0 to avoid a fuzz timeout there, but it isn't too important, up to the backport maintainer)
(https://github.com/bitcoin/bitcoin/pull/31917#issuecomment-2726515276)
(In theory this could be backported to 29.0 to avoid a fuzz timeout there, but it isn't too important, up to the backport maintainer)
⚠️ bearycool11 opened an issue: "Feature Request: Forking Bitcoin for Native gRPC/dRPC Integration"
(https://github.com/bitcoin/bitcoin/issues/32076)
### Please describe the feature you'd like to see added.
So I noticed on mintscan that we have bitcoin API, but... we don't have gRPC/dRPC hooking in bitcore, which is causing a lot of wacky stuff to occur on Coinbase.
Then again, if we're going to introduce this feature we to be careful of this following bug vulnerability
interface ProviderRpcError {
message: string;
code: number;
data?: { method?: string };
stack?: string;
}
Because we all know what that does. Coinbase, the cryp
...
(https://github.com/bitcoin/bitcoin/issues/32076)
### Please describe the feature you'd like to see added.
So I noticed on mintscan that we have bitcoin API, but... we don't have gRPC/dRPC hooking in bitcore, which is causing a lot of wacky stuff to occur on Coinbase.
Then again, if we're going to introduce this feature we to be careful of this following bug vulnerability
interface ProviderRpcError {
message: string;
code: number;
data?: { method?: string };
stack?: string;
}
Because we all know what that does. Coinbase, the cryp
...