💬 fjahr commented on pull request "Prevent file descriptor exhaustion from too many RPC calls":
(https://github.com/bitcoin/bitcoin/pull/27731#discussion_r1245435268)
Hm, how would that happen? I didn't have any issues when testing this code with 2.2.1-alpha and 2.1.12-stable.
(https://github.com/bitcoin/bitcoin/pull/27731#discussion_r1245435268)
Hm, how would that happen? I didn't have any issues when testing this code with 2.2.1-alpha and 2.1.12-stable.
📝 sipa opened a pull request: "Make poly1305 support incremental computation + modernize"
(https://github.com/bitcoin/bitcoin/pull/27993)
Our current Poly1305 code (src/crypto/poly1305.*) only supports computing the entire tag in one go (the `poly1305_auth` function takes a key and message, and outputs the tag). However, the RFC8439 authenticated encryption (as used in BIP324, see #27634) scheme makes use of Poly1305 in a way where the message consists of 3 different pieces:
* The additionally authenticated data (AAD), padded to 16 bytes.
* The ciphertext, padded to 16 bytes.
* The length of the AAD and the length of the cipher
...
(https://github.com/bitcoin/bitcoin/pull/27993)
Our current Poly1305 code (src/crypto/poly1305.*) only supports computing the entire tag in one go (the `poly1305_auth` function takes a key and message, and outputs the tag). However, the RFC8439 authenticated encryption (as used in BIP324, see #27634) scheme makes use of Poly1305 in a way where the message consists of 3 different pieces:
* The additionally authenticated data (AAD), padded to 16 bytes.
* The ciphertext, padded to 16 bytes.
* The length of the AAD and the length of the cipher
...
🤔 jonatack reviewed a pull request: "test: remove race in the user-agent reception check"
(https://github.com/bitcoin/bitcoin/pull/27986#pullrequestreview-1503489729)
Light tested ACK c1a169082ac1147ec2f3cc329f23bc0bfc28cd6b
(https://github.com/bitcoin/bitcoin/pull/27986#pullrequestreview-1503489729)
Light tested ACK c1a169082ac1147ec2f3cc329f23bc0bfc28cd6b
💬 jonatack commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1245457894)
Suggest while touching this, to make it clearer and more coherent with the documentation just above.
```diff
- # Consistency check that the Bitcoin Core has received our user agent string.
- # Find our connection in "getpeerinfo" by our address:port, it is unique.
+ # Consistency check that the node received our user agent string.
+ # Find our connection in getpeerinfo by our address:port, as it is unique.
```
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1245457894)
Suggest while touching this, to make it clearer and more coherent with the documentation just above.
```diff
- # Consistency check that the Bitcoin Core has received our user agent string.
- # Find our connection in "getpeerinfo" by our address:port, it is unique.
+ # Consistency check that the node received our user agent string.
+ # Find our connection in getpeerinfo by our address:port, as it is unique.
```
💬 vasild commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#issuecomment-1611751064)
`c1a169082a...28d26c4f37`: take https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1245457894
(https://github.com/bitcoin/bitcoin/pull/27986#issuecomment-1611751064)
`c1a169082a...28d26c4f37`: take https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1245457894
💬 vasild commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1245488264)
Done.
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1245488264)
Done.
💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1245477180)
In commit "refactor: simplify pruning violation check" (2412ffdce69ce2b164e5303ae4b610ab0d00fd5c)
Looking at this code, it's not really clear that `block_to_test` is going to be non-null, and that the `CheckBlockDataAvailability` check is going to pass. If it were null, the pruning check would fail even if there were no pruned data.
So I think it would be good add an assert with an explanation of why `block_to_test` can't be null here, and I implemented this below.
While implementing th
...
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1245477180)
In commit "refactor: simplify pruning violation check" (2412ffdce69ce2b164e5303ae4b610ab0d00fd5c)
Looking at this code, it's not really clear that `block_to_test` is going to be non-null, and that the `CheckBlockDataAvailability` check is going to pass. If it were null, the pruning check would fail even if there were no pruned data.
So I think it would be good add an assert with an explanation of why `block_to_test` can't be null here, and I implemented this below.
While implementing th
...
👍 ryanofsky approved a pull request: "index: make startup more efficient"
(https://github.com/bitcoin/bitcoin/pull/27607#pullrequestreview-1503519460)
Code review ACK 2f830d1de8f0b57f46f1d7da3cb7b9ab4aa1e2ff. I left a long comment below about one of the intermediate commits after I got hung up on whether `block_to_test` could be null before it was passed to the CheckBlockData function. I think it is not too important since the code is deleted in the end, but I did suggest some improvements to CheckBlockData that I think would make it safer and easier to use.
(https://github.com/bitcoin/bitcoin/pull/27607#pullrequestreview-1503519460)
Code review ACK 2f830d1de8f0b57f46f1d7da3cb7b9ab4aa1e2ff. I left a long comment below about one of the intermediate commits after I got hung up on whether `block_to_test` could be null before it was passed to the CheckBlockData function. I think it is not too important since the code is deleted in the end, but I did suggest some improvements to CheckBlockData that I think would make it safer and easier to use.
👍 jamesob approved a pull request: "test: Fuzz on macOS"
(https://github.com/bitcoin/bitcoin/pull/27932#pullrequestreview-1503552896)
Github ACK https://github.com/bitcoin/bitcoin/pull/27932/commits/fae7c50d201726f605938c3511dd9119efeea5ec
Adds fuzzing to a new architecture, right? Seems like an easy win?
(https://github.com/bitcoin/bitcoin/pull/27932#pullrequestreview-1503552896)
Github ACK https://github.com/bitcoin/bitcoin/pull/27932/commits/fae7c50d201726f605938c3511dd9119efeea5ec
Adds fuzzing to a new architecture, right? Seems like an easy win?
💬 jamesob commented on pull request "util: Don't derive secure_allocator from std::allocator":
(https://github.com/bitcoin/bitcoin/pull/27930#issuecomment-1611769447)
Concept ACK. Thanks for filing the change @CaseyCarter, and welcome to the project!
(https://github.com/bitcoin/bitcoin/pull/27930#issuecomment-1611769447)
Concept ACK. Thanks for filing the change @CaseyCarter, and welcome to the project!
👍 jamesob approved a pull request: "test: Fix intermittent issue in mining_getblocktemplate_longpoll.py"
(https://github.com/bitcoin/bitcoin/pull/27941#pullrequestreview-1503585748)
Github ACK https://github.com/bitcoin/bitcoin/pull/27941/commits/fa748c6f2ac2f9cac7ce42fd745ed3c3eae093b7
Seems like a commonsense fix? Can't test the RPC method if the Bitcoin Core HTTP handler hasn't booted.
(https://github.com/bitcoin/bitcoin/pull/27941#pullrequestreview-1503585748)
Github ACK https://github.com/bitcoin/bitcoin/pull/27941/commits/fa748c6f2ac2f9cac7ce42fd745ed3c3eae093b7
Seems like a commonsense fix? Can't test the RPC method if the Bitcoin Core HTTP handler hasn't booted.
👍 vasild approved a pull request: "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting"
(https://github.com/bitcoin/bitcoin/pull/27411#pullrequestreview-1503590039)
ACK e7cf8657e1165ea5da3911a9e543837cd8938f97
(https://github.com/bitcoin/bitcoin/pull/27411#pullrequestreview-1503590039)
ACK e7cf8657e1165ea5da3911a9e543837cd8938f97
💬 jonatack commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1245517426)
Verified per https://docs.python.org/3/whatsnew/3.8.html that [asyncio.BaseTransport.get_extra_info()](https://docs.python.org/3/library/asyncio-protocol.html#asyncio.BaseTransport.get_extra_info) was added in Python 3.8, the minimum supported version per `doc/dependencies.md`.
I'm not sure if `import asyncio` should be added to this file.
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1245517426)
Verified per https://docs.python.org/3/whatsnew/3.8.html that [asyncio.BaseTransport.get_extra_info()](https://docs.python.org/3/library/asyncio-protocol.html#asyncio.BaseTransport.get_extra_info) was added in Python 3.8, the minimum supported version per `doc/dependencies.md`.
I'm not sure if `import asyncio` should be added to this file.
💬 jonatack commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#issuecomment-1611783471)
ACK 28d26c4f37c433e35a4a05e144d05f0e464f8452
modulo question in https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1245517426
(https://github.com/bitcoin/bitcoin/pull/27986#issuecomment-1611783471)
ACK 28d26c4f37c433e35a4a05e144d05f0e464f8452
modulo question in https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1245517426
💬 ErikDeSmedt commented on pull request "wallet: bugfix, always use apostrophe for spkm descriptor ID":
(https://github.com/bitcoin/bitcoin/pull/27920#issuecomment-1611795857)
ACK: I've tested the changes and the bug is resolved.
I've been struggling with back-porting the test-code to v25.0.
You might need to change some descriptors to use `h` instead of `'` and I manually removed some checksums.
@Sjors : Thanks for helping me out with this
(https://github.com/bitcoin/bitcoin/pull/27920#issuecomment-1611795857)
ACK: I've tested the changes and the bug is resolved.
I've been struggling with back-porting the test-code to v25.0.
You might need to change some descriptors to use `h` instead of `'` and I manually removed some checksums.
@Sjors : Thanks for helping me out with this
📝 Brotcrunsher opened a pull request: "DRAFT: Checking for multi/single-value types in UniValue."
(https://github.com/bitcoin/bitcoin/pull/27994)
Previously it was possible to call getValStr() on a VOBJ and VARR, which silently resulted in the return of an empty string. However, such a call would be most likely a bug. We are now throwing in case of such types.
Similarly, calling empty() and size() didn't make sense for none VOBJ/VARR UniValues.
**Please note:** This is a draft and not meant to be merged just yet. I created the pull request already because I wanted to see what the CI-Pipeline tells me about this. I also want to give
...
(https://github.com/bitcoin/bitcoin/pull/27994)
Previously it was possible to call getValStr() on a VOBJ and VARR, which silently resulted in the return of an empty string. However, such a call would be most likely a bug. We are now throwing in case of such types.
Similarly, calling empty() and size() didn't make sense for none VOBJ/VARR UniValues.
**Please note:** This is a draft and not meant to be merged just yet. I created the pull request already because I wanted to see what the CI-Pipeline tells me about this. I also want to give
...
⚠️ sipa opened an issue: "Improving fee estimation accuracy"
(https://github.com/bitcoin/bitcoin/issues/27995)
The current fee estimation algorithm suffers from a number of issues that make it hard to use in the real world:
* Because it's solely based on historical data (looking at how long mempool transactions take to confirm), it cannot react quickly to changing conditions.
* Because it's aiming to match seen behavior rather than requirement, if some non-negligible fraction of users keeps paying a certain high feerate, it may try to match that, even if unnecessary for confirmation.
There exist alt
...
(https://github.com/bitcoin/bitcoin/issues/27995)
The current fee estimation algorithm suffers from a number of issues that make it hard to use in the real world:
* Because it's solely based on historical data (looking at how long mempool transactions take to confirm), it cannot react quickly to changing conditions.
* Because it's aiming to match seen behavior rather than requirement, if some non-negligible fraction of users keeps paying a certain high feerate, it may try to match that, even if unnecessary for confirmation.
There exist alt
...
💬 sipa commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1611839519)
I think the discussion here is conflating two things:
* Problems with Bitcoin Core's fee estimation algorithm.
* The lack of a fee histogram RPC.
It's clear that the existing fee estimation algorithms make it inaccurate for many real use cases. There are good reasons for why it works the way it does, but that doesn't change the fact that the outcome is bad. We can certainly discuss what can be done about that. I've posted about one possible approach in #27995.
However, this PR is about s
...
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1611839519)
I think the discussion here is conflating two things:
* Problems with Bitcoin Core's fee estimation algorithm.
* The lack of a fee histogram RPC.
It's clear that the existing fee estimation algorithms make it inaccurate for many real use cases. There are good reasons for why it works the way it does, but that doesn't change the fact that the outcome is bad. We can certainly discuss what can be done about that. I've posted about one possible approach in #27995.
However, this PR is about s
...
💬 sipa commented on pull request "test: add python implementation of Elligator swift":
(https://github.com/bitcoin/bitcoin/pull/24005#issuecomment-1611844399)
@stratospher Want to address the comments here, and rebase? It'd be good to get rid of the merge commit in this PR's history.
(https://github.com/bitcoin/bitcoin/pull/24005#issuecomment-1611844399)
@stratospher Want to address the comments here, and rebase? It'd be good to get rid of the merge commit in this PR's history.
🤔 john-moffett reviewed a pull request: "util: Don't derive secure_allocator from std::allocator"
(https://github.com/bitcoin/bitcoin/pull/27930#pullrequestreview-1503703565)
Concept ACK and existing code ACK. Thanks for catching this!
I agree that `zero_after_free_allocator` also ought to be modified. FWIW, here's what I did:
```diff
diff --git a/src/support/allocators/zeroafterfree.h b/src/support/allocators/zeroafterfree.h
index 2dc644c242a..35312e9704d 100644
--- a/src/support/allocators/zeroafterfree.h
+++ b/src/support/allocators/zeroafterfree.h
@@ -12,31 +12,46 @@
#include <vector>
template <typename T>
-struct zero_after_free_allocator : pu
...
(https://github.com/bitcoin/bitcoin/pull/27930#pullrequestreview-1503703565)
Concept ACK and existing code ACK. Thanks for catching this!
I agree that `zero_after_free_allocator` also ought to be modified. FWIW, here's what I did:
```diff
diff --git a/src/support/allocators/zeroafterfree.h b/src/support/allocators/zeroafterfree.h
index 2dc644c242a..35312e9704d 100644
--- a/src/support/allocators/zeroafterfree.h
+++ b/src/support/allocators/zeroafterfree.h
@@ -12,31 +12,46 @@
#include <vector>
template <typename T>
-struct zero_after_free_allocator : pu
...