💬 achow101 commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1570654857)
@ryanofsky Are you planning on making any further changes here?
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1570654857)
@ryanofsky Are you planning on making any further changes here?
💬 sr-gi commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1570662645)
So just to be clear, my concern here relates to the fact that the current codebase only allows a peer to request a transaction as long as we have announced that to them (with the exception of data requested after a `MEMPOOL` message if we happen run signaling bip37).
After this patch, the logic above won't hold true anymore. If for whatever reason we have more than `INV_BROADCAST_MAX` transactions to send to a given peer at a given time, and we send an INV message to it, it will immediately b
...
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1570662645)
So just to be clear, my concern here relates to the fact that the current codebase only allows a peer to request a transaction as long as we have announced that to them (with the exception of data requested after a `MEMPOOL` message if we happen run signaling bip37).
After this patch, the logic above won't hold true anymore. If for whatever reason we have more than `INV_BROADCAST_MAX` transactions to send to a given peer at a given time, and we send an INV message to it, it will immediately b
...
💬 Crypt-iQ commented on issue "bitcoind hangs waiting for `g_requests.empty()`":
(https://github.com/bitcoin/bitcoin/issues/27722#issuecomment-1570668159)
I get this when compiling with both gcc and clang-16. I am also using libevent 2.1.12. Is there anything unique about your setup @willcl-ark ? Also, did you Ctrl-C the `waitforblockheight` command before trying to stop bitcoind?
(https://github.com/bitcoin/bitcoin/issues/27722#issuecomment-1570668159)
I get this when compiling with both gcc and clang-16. I am also using libevent 2.1.12. Is there anything unique about your setup @willcl-ark ? Also, did you Ctrl-C the `waitforblockheight` command before trying to stop bitcoind?
🚀 achow101 merged a pull request: "index: prevent race by calling 'CustomInit' prior setting 'synced' flag"
(https://github.com/bitcoin/bitcoin/pull/27720)
(https://github.com/bitcoin/bitcoin/pull/27720)
💬 stratospher commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212094169)
8990668: it would be useful to have radd and rsub back for [these operations in `xswiftec_inv`](https://github.com/bitcoin/bips/blob/master/bip-0324/reference.py#L367-L370).
(to keep #24005 consistent with BIP's reference python implementation for easy review.)
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212094169)
8990668: it would be useful to have radd and rsub back for [these operations in `xswiftec_inv`](https://github.com/bitcoin/bips/blob/master/bip-0324/reference.py#L367-L370).
(to keep #24005 consistent with BIP's reference python implementation for easy review.)
💬 sipa commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212129551)
I mean that the computation won't be repeated if called a second time (due to self.den being set to 1, which triggers the fast path). Whether that's accomplished through actually remembering the int-converted form or some other mechanism is an implementation detail a user of the class shouldn't need to care about.
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212129551)
I mean that the computation won't be repeated if called a second time (due to self.den being set to 1, which triggers the fast path). Whether that's accomplished through actually remembering the int-converted form or some other mechanism is an implementation detail a user of the class shouldn't need to care about.
💬 MarcoFalke commented on pull request "Add public Boost headers explicitly":
(https://github.com/bitcoin/bitcoin/pull/27783#issuecomment-1570717706)
lgtm ACK 2484cacb7a6367b24e924dba0825c843b1dfc1c3
(https://github.com/bitcoin/bitcoin/pull/27783#issuecomment-1570717706)
lgtm ACK 2484cacb7a6367b24e924dba0825c843b1dfc1c3
🤔 Sjors reviewed a pull request: "Draft: rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1453921658)
If you can split 534b6f99a3779465678f39ed2704da6049c6469d in ~half that would make it a bit easier to review. Maybe start with anything that's trivial to move, and then do the more involved changes in the second commit.
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1453921658)
If you can split 534b6f99a3779465678f39ed2704da6049c6469d in ~half that would make it a bit easier to review. Maybe start with anything that's trivial to move, and then do the more involved changes in the second commit.
💬 Sjors commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1212154654)
534b6f99a3779465678f39ed2704da6049c6469d: this function seems to come out of nowhere and is only used in a test.
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1212154654)
534b6f99a3779465678f39ed2704da6049c6469d: this function seems to come out of nowhere and is only used in a test.
💬 ryanofsky commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1570744518)
> @ryanofsky Are you planning on making any further changes here?
I'm not working on anything. The only suggestion I saw was to rename the `fr` variable, but that is a preexisting variable so would prefer not to change that here.
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1570744518)
> @ryanofsky Are you planning on making any further changes here?
I'm not working on anything. The only suggestion I saw was to rename the `fr` variable, but that is a preexisting variable so would prefer not to change that here.
💬 sipa commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212163089)
I guess we should; the parentheses are for listing parent classes, but I believe that an empty list or no parentheses at all don't make a difference.
Fixed.
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212163089)
I guess we should; the parentheses are for listing parent classes, but I believe that an empty list or no parentheses at all don't make a difference.
Fixed.
💬 sipa commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212164841)
I've added a `"no overflow allowed"` to the comment.
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212164841)
I've added a `"no overflow allowed"` to the comment.
💬 sipa commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212166978)
I've kept the `FE.is_square()` function, but replaced it with just a `x.sqrt() is not None` plus a comment that a more efficient algorithm is possible.
This leaves the option of easily adding a more efficient implementation back later if really worth it.
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212166978)
I've kept the `FE.is_square()` function, but replaced it with just a `x.sqrt() is not None` plus a comment that a more efficient algorithm is possible.
This leaves the option of easily adding a more efficient implementation back later if really worth it.
💬 sipa commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212163658)
Ok, I added back.
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212163658)
Ok, I added back.
💬 sipa commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212172722)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212172722)
Fixed.
💬 sipa commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#issuecomment-1570755789)
Addressed review comments. I've also renamed `FE.num` and `FE.den` to `FE._num` and `FE._den` to mark them as private (to the extent that Python allows that). All interactions with these objects should be done through their methods.
(https://github.com/bitcoin/bitcoin/pull/26222#issuecomment-1570755789)
Addressed review comments. I've also renamed `FE.num` and `FE.den` to `FE._num` and `FE._den` to mark them as private (to the extent that Python allows that). All interactions with these objects should be done through their methods.
💬 achow101 commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1570774397)
Ah okay, I wasn't sure if you were going to add the named-only parameters to the conversion table, but based on the above discussion, it sounds like we're going to move towards eliminating the table in a followup.
ACK 2808c33bae95a0d2516a6e9a550032af142a04de
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1570774397)
Ah okay, I wasn't sure if you were going to add the named-only parameters to the conversion table, but based on the above discussion, it sounds like we're going to move towards eliminating the table in a followup.
ACK 2808c33bae95a0d2516a6e9a550032af142a04de
💬 Sjors commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1212164200)
534b6f99a3779465678f39ed2704da6049c6469d if it's broken in master, can we fix it _before_ refactoring? Alternatively there could be a `This will be fixed in the next commit` comment.
I'm also confused by the comment. `TryAddBlockIndexCandidate` is only called from `ReceivedBlockTransactions` after it sets `BLOCK_VALID_TRANSACTIONS`. So are you saying we're currently too strict? Was there a regression somewhere?
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1212164200)
534b6f99a3779465678f39ed2704da6049c6469d if it's broken in master, can we fix it _before_ refactoring? Alternatively there could be a `This will be fixed in the next commit` comment.
I'm also confused by the comment. `TryAddBlockIndexCandidate` is only called from `ReceivedBlockTransactions` after it sets `BLOCK_VALID_TRANSACTIONS`. So are you saying we're currently too strict? Was there a regression somewhere?
💬 Sjors commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1212161483)
534b6f99a3779465678f39ed2704da6049c6469d: maybe use this refactor as an opportunity to add a comment explaining the somewhat obtuse line above
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1212161483)
534b6f99a3779465678f39ed2704da6049c6469d: maybe use this refactor as an opportunity to add a comment explaining the somewhat obtuse line above
💬 Sjors commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1212173301)
534b6f99a3779465678f39ed2704da6049c6469d: `"active" :"background"`?
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1212173301)
534b6f99a3779465678f39ed2704da6049c6469d: `"active" :"background"`?