🤔 theStack reviewed a pull request: "crypto: add `NUMS_H` const"
(https://github.com/bitcoin/bitcoin/pull/30048#pullrequestreview-2040638390)
Concept ACK
Found via `$ git grep 5092929b74` that we already define this byte-string constant twice for the miniscript fuzz and unit tests (`src/test/fuzz/miniscript.cpp` and `src/test/miniscript_tests.cpp`, called `NUMS_PK` there), that's a good opportunity to deduplicate.
(https://github.com/bitcoin/bitcoin/pull/30048#pullrequestreview-2040638390)
Concept ACK
Found via `$ git grep 5092929b74` that we already define this byte-string constant twice for the miniscript fuzz and unit tests (`src/test/fuzz/miniscript.cpp` and `src/test/miniscript_tests.cpp`, called `NUMS_PK` there), that's a good opportunity to deduplicate.
💬 josibake commented on pull request "Compare `COutPoints` lexicographically":
(https://github.com/bitcoin/bitcoin/pull/30046#discussion_r1590976847)
Yep, good point. I added a comment to clarify that this is a lexicographic sort on the serialized outpoint. I think this is preferable to what we were doing before since its based on the protocol definition of an outpoint: `<32-byte txid, little-endian>:<4-byte vout, little-endian>`.
(https://github.com/bitcoin/bitcoin/pull/30046#discussion_r1590976847)
Yep, good point. I added a comment to clarify that this is a lexicographic sort on the serialized outpoint. I think this is preferable to what we were doing before since its based on the protocol definition of an outpoint: `<32-byte txid, little-endian>:<4-byte vout, little-endian>`.
💬 josibake commented on pull request "Compare `COutPoints` lexicographically":
(https://github.com/bitcoin/bitcoin/pull/30046#issuecomment-2095964573)
> Don't forget to add a source comment that BIP352 depends on this exact ordering. Either in this PR or as a commit in #28122.
Hm, I don't think we need a BIP352 specific comment here? I'd argue if we are going to sort outpoints, we should be sorting them based on their protocol definition, regardless of BIP352. In fact, when writing BIP352 we already assumed this was the case which is why we defined the outpoint sorting in BIP352 to be done lexicographically on the outpoint serialization. It
...
(https://github.com/bitcoin/bitcoin/pull/30046#issuecomment-2095964573)
> Don't forget to add a source comment that BIP352 depends on this exact ordering. Either in this PR or as a commit in #28122.
Hm, I don't think we need a BIP352 specific comment here? I'd argue if we are going to sort outpoints, we should be sorting them based on their protocol definition, regardless of BIP352. In fact, when writing BIP352 we already assumed this was the case which is why we defined the outpoint sorting in BIP352 to be done lexicographically on the outpoint serialization. It
...
🚀 fanquake merged a pull request: "build, bench, msvc: Add missing benchmarks"
(https://github.com/bitcoin/bitcoin/pull/29773)
(https://github.com/bitcoin/bitcoin/pull/29773)
💬 josibake commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#issuecomment-2095990357)
> Found via `$ git grep 5092929b74`
Nice! Pushed a second commit to update the existing tests.
(https://github.com/bitcoin/bitcoin/pull/30048#issuecomment-2095990357)
> Found via `$ git grep 5092929b74`
Nice! Pushed a second commit to update the existing tests.
💬 Cinnamonrou commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1591001090)
- [ ] - []()~~~~
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1591001090)
- [ ] - []()~~~~
💬 Cinnamonrou commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1591000672)
- [ ]
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1591000672)
- [ ]
💬 sipa commented on pull request "Compare `COutPoints` lexicographically":
(https://github.com/bitcoin/bitcoin/pull/30046#issuecomment-2096017270)
This seems unnecessarily unnatural and inefficient to use as a default sort order for this type.
If BIP352 defines its own normative sort order, I think it's better to have a specialized BIP352Comparator class that implements it, and use that in the BIP352 code?
(https://github.com/bitcoin/bitcoin/pull/30046#issuecomment-2096017270)
This seems unnecessarily unnatural and inefficient to use as a default sort order for this type.
If BIP352 defines its own normative sort order, I think it's better to have a specialized BIP352Comparator class that implements it, and use that in the BIP352 code?
💬 sr-gi commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1591046220)
I've been trying to reproduce in my local setup but I'm unable. Did you manage to trigger this on your updated branch, or with this one?
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1591046220)
I've been trying to reproduce in my local setup but I'm unable. Did you manage to trigger this on your updated branch, or with this one?
💬 sr-gi commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1591045131)
Yep
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1591045131)
Yep
💬 fjahr commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2096075601)
Added @wiz as the second seed node 🚀
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2096075601)
Added @wiz as the second seed node 🚀
💬 fjahr commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2096079112)
> I don't think that's necessary. However, you could use a recent block hash as a provably unspendable public key (instead of all zeros).
Well, we now have a genesis block with the hash in the message and the all zeros public key. I don't think that makes a difference anymore?
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2096079112)
> I don't think that's necessary. However, you could use a recent block hash as a provably unspendable public key (instead of all zeros).
Well, we now have a genesis block with the hash in the message and the all zeros public key. I don't think that makes a difference anymore?
💬 jlopp commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2096133300)
I consider closing the block storm loophole to be rather important - it's the primary reason why TBTC became scarce and accrued value far faster than ought to have happened. Given that the patch is rather trivial, it would be a huge missed opportunity not to fix it at this time.
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2096133300)
I consider closing the block storm loophole to be rather important - it's the primary reason why TBTC became scarce and accrued value far faster than ought to have happened. Given that the patch is rather trivial, it would be a huge missed opportunity not to fix it at this time.
💬 fjahr commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2096167567)
> I consider closing the block storm loophole to be rather important - it's the primary reason why TBTC became scarce and accrued value far faster than ought to have happened. Given that the patch is rather trivial, it would be a huge missed opportunity not to fix it at this time.
I agree. Could you say precisely which (combination) of the [ideas given here so far](https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2094300149) do you consider a fix? Currently this PR includes the fix
...
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2096167567)
> I consider closing the block storm loophole to be rather important - it's the primary reason why TBTC became scarce and accrued value far faster than ought to have happened. Given that the patch is rather trivial, it would be a huge missed opportunity not to fix it at this time.
I agree. Could you say precisely which (combination) of the [ideas given here so far](https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2094300149) do you consider a fix? Currently this PR includes the fix
...
💬 luke-jr commented on pull request "refactor: Simply include CTxMemPool::Options in CTxMemPool directly rather than duplicating definition":
(https://github.com/bitcoin/bitcoin/pull/29086#issuecomment-2096168450)
reopen please
(https://github.com/bitcoin/bitcoin/pull/29086#issuecomment-2096168450)
reopen please
📝 fanquake reopened a pull request: "refactor: Simply include CTxMemPool::Options in CTxMemPool directly rather than duplicating definition"
(https://github.com/bitcoin/bitcoin/pull/29086)
Instead of duplicating mempool options two places, just include the Options struct directly on the CTxMemPool
(https://github.com/bitcoin/bitcoin/pull/29086)
Instead of duplicating mempool options two places, just include the Options struct directly on the CTxMemPool
💬 sr-gi commented on pull request "net: don't lock cs_main while reading blocks in net processing":
(https://github.com/bitcoin/bitcoin/pull/26326#issuecomment-2096174963)
ACK [75d27fe](https://github.com/bitcoin/bitcoin/pull/26326/commits/75d27fefc7a04ebdda7be5724a014b6a896e7325)
The only differences are the ones mentioned, which align with the approach after considering https://github.com/bitcoin/bitcoin/pull/26326#discussion_r1588242121
(https://github.com/bitcoin/bitcoin/pull/26326#issuecomment-2096174963)
ACK [75d27fe](https://github.com/bitcoin/bitcoin/pull/26326/commits/75d27fefc7a04ebdda7be5724a014b6a896e7325)
The only differences are the ones mentioned, which align with the approach after considering https://github.com/bitcoin/bitcoin/pull/26326#discussion_r1588242121
👍 ryanofsky approved a pull request: "[refactor] Check CTxMemPool options in ctor"
(https://github.com/bitcoin/bitcoin/pull/28830#pullrequestreview-2040944697)
Code review ACK d447bdcfb0e38353940e4a7fc89d09482d8d39c3. Thanks for the updates!
(https://github.com/bitcoin/bitcoin/pull/28830#pullrequestreview-2040944697)
Code review ACK d447bdcfb0e38353940e4a7fc89d09482d8d39c3. Thanks for the updates!
✅ josibake closed a pull request: "Compare `COutPoints` lexicographically"
(https://github.com/bitcoin/bitcoin/pull/30046)
(https://github.com/bitcoin/bitcoin/pull/30046)
💬 josibake commented on pull request "Compare `COutPoints` lexicographically":
(https://github.com/bitcoin/bitcoin/pull/30046#issuecomment-2096195301)
> This seems unnecessarily unnatural and inefficient to use as a default sort order for this type.
>
> If BIP352 defines its own normative sort order, I think it's better to have a specialized BIP352Comparator class that implements it, and use that in the BIP352 code?
>
> That also has the advantage of not tying the two together; someone might come in and change the order back to the current ordering for performance or simplicity reasons, and break the BIP352 code.
Personally, I find it
...
(https://github.com/bitcoin/bitcoin/pull/30046#issuecomment-2096195301)
> This seems unnecessarily unnatural and inefficient to use as a default sort order for this type.
>
> If BIP352 defines its own normative sort order, I think it's better to have a specialized BIP352Comparator class that implements it, and use that in the BIP352 code?
>
> That also has the advantage of not tying the two together; someone might come in and change the order back to the current ordering for performance or simplicity reasons, and break the BIP352 code.
Personally, I find it
...