💬 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
...
💬 jlopp commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2096201740)
I like all of the currently proposed changes.
So the question is this: if a high hashrate miner deliberately stalls testnet and we limp along for an entire difficulty adjustment period with minimum difficulty blocks, what should happen?
I think, in that case, perhaps we SHOULD reset the actual difficulty to the minimum difficulty (or to a sane ASIC difficulty like 1,000,000) and allow the network to find the real hashrate again.
Thus, it would still technically be possible for a block s
...
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2096201740)
I like all of the currently proposed changes.
So the question is this: if a high hashrate miner deliberately stalls testnet and we limp along for an entire difficulty adjustment period with minimum difficulty blocks, what should happen?
I think, in that case, perhaps we SHOULD reset the actual difficulty to the minimum difficulty (or to a sane ASIC difficulty like 1,000,000) and allow the network to find the real hashrate again.
Thus, it would still technically be possible for a block s
...
💬 mzumsande commented on issue "RfC: increase minimum prune target?":
(https://github.com/bitcoin/bitcoin/issues/30037#issuecomment-2096202995)
> since nodes with limited space would believe bitcoind was being limited/constrained to a particular allocated amount of space but would be exceeding the allocated amount
This is already the case today, because the 288 blocks rule takes precedence over the user-provided number. With average block sizes of 1.6MiB currently, my synced `prune=550` node usually takes up between 700MiB and 900MiB of space for `*.blk` and `*.rev` files.. The target is only respected when possible (during the earli
...
(https://github.com/bitcoin/bitcoin/issues/30037#issuecomment-2096202995)
> since nodes with limited space would believe bitcoind was being limited/constrained to a particular allocated amount of space but would be exceeding the allocated amount
This is already the case today, because the 288 blocks rule takes precedence over the user-provided number. With average block sizes of 1.6MiB currently, my synced `prune=550` node usually takes up between 700MiB and 900MiB of space for `*.blk` and `*.rev` files.. The target is only respected when possible (during the earli
...
💬 mzumsande commented on issue "Manually Banning Peers Results in Crash":
(https://github.com/bitcoin/bitcoin/issues/29916#issuecomment-2096263893)
> Could this be a race between "clicking on the peer to ban it in the GUI" and "the peer disconnecting"?
I thought the same last week and tried to reproduce it with node by adding a sleep after a node disconnected, but didn't manage to reproduce the crash (I might have put the sleep in the wrong place though...).
(https://github.com/bitcoin/bitcoin/issues/29916#issuecomment-2096263893)
> Could this be a race between "clicking on the peer to ban it in the GUI" and "the peer disconnecting"?
I thought the same last week and tried to reproduce it with node by adding a sleep after a node disconnected, but didn't manage to reproduce the crash (I might have put the sleep in the wrong place though...).
💬 pinheadmz commented on issue "release: ship codesigned MacOS arm64 binaries":
(https://github.com/bitcoin/bitcoin/issues/29749#issuecomment-2096273770)
I think this is a decent idea. I am a [macOS codesigner](https://github.com/bitcoin-core/bitcoin-detached-sigs/pull/34) and was able to sign a guix-built `bin/bitcoind` release as well as create a detached signature like we do for the gui. However I did so with the Apple-blessed tools (OS keychain and `codesign` command). I was also unable to re-attach that signature to the binary which would be required for guix attestations. So the work involved here would mostly be patching https://github.com
...
(https://github.com/bitcoin/bitcoin/issues/29749#issuecomment-2096273770)
I think this is a decent idea. I am a [macOS codesigner](https://github.com/bitcoin-core/bitcoin-detached-sigs/pull/34) and was able to sign a guix-built `bin/bitcoind` release as well as create a detached signature like we do for the gui. However I did so with the Apple-blessed tools (OS keychain and `codesign` command). I was also unable to re-attach that signature to the binary which would be required for guix attestations. So the work involved here would mostly be patching https://github.com
...
💬 pinheadmz commented on pull request "rpc, wallet: fix incorrect segwit redeem script size limit":
(https://github.com/bitcoin/bitcoin/pull/28307#issuecomment-2096308420)
> The confusing point about this command is that it returns the post-processed tx hex regardless of what it was able to do internally or if the script passes the verification step. This is because, in a multisig scenario, the process could have appended only one of the signatures, which would obviously fail during the script verification. Thus, the process needs to update the transaction and notify the user that there are still missing signatures so it can relay it to the next party.
Ah thank
...
(https://github.com/bitcoin/bitcoin/pull/28307#issuecomment-2096308420)
> The confusing point about this command is that it returns the post-processed tx hex regardless of what it was able to do internally or if the script passes the verification step. This is because, in a multisig scenario, the process could have appended only one of the signatures, which would obviously fail during the script verification. Thus, the process needs to update the transaction and notify the user that there are still missing signatures so it can relay it to the next party.
Ah thank
...
💬 mannyg253 commented on pull request "Compare `COutPoints` lexicographically":
(https://github.com/bitcoin/bitcoin/pull/30046#issuecomment-2096315155)
Bitcoin
(https://github.com/bitcoin/bitcoin/pull/30046#issuecomment-2096315155)
Bitcoin
💬 instagibbs commented on pull request "rpc: improve submitpackage documentation and other improvements":
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1591182646)
found it weird
```Suggestion
assert_raises_rpc_error(-8, f"Array must contain between 1 and {MAX_PACKAGE_COUNT} transactions.", node.submitpackage, [])
```
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1591182646)
found it weird
```Suggestion
assert_raises_rpc_error(-8, f"Array must contain between 1 and {MAX_PACKAGE_COUNT} transactions.", node.submitpackage, [])
```
💬 instagibbs commented on pull request "rpc: improve submitpackage documentation and other improvements":
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1591189552)
if we're lining up ala `testmempoolaccept` shouldn't we be adding single quotes around the whole array as well?
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1591189552)
if we're lining up ala `testmempoolaccept` shouldn't we be adding single quotes around the whole array as well?
👍 pinheadmz approved a pull request: "rpc, wallet: fix incorrect segwit redeem script size limit"
(https://github.com/bitcoin/bitcoin/pull/28307#pullrequestreview-2041087253)
ACK 2451a217dd2c21b6d2f2b2699ceddd0bf9073019
Minimal diff since last ACK, just rebase on master and replace a"520" with a "MAX_SCRIPT_ELEMENT_SIZE"
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 2451a217dd2c21b6d2f2b2699ceddd0bf9073019
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmY4+ZIACgkQ5+KYS2KJ
yTqhnhAApaBq0dhQVXvLaTSBjV8Q8CXJTXapgme6uII649DlRjCQujM4ufmnPfdR
BuQWzAC5FbgA1sCkRk3+D/CyO0H9k5PP
...
(https://github.com/bitcoin/bitcoin/pull/28307#pullrequestreview-2041087253)
ACK 2451a217dd2c21b6d2f2b2699ceddd0bf9073019
Minimal diff since last ACK, just rebase on master and replace a"520" with a "MAX_SCRIPT_ELEMENT_SIZE"
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 2451a217dd2c21b6d2f2b2699ceddd0bf9073019
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmY4+ZIACgkQ5+KYS2KJ
yTqhnhAApaBq0dhQVXvLaTSBjV8Q8CXJTXapgme6uII649DlRjCQujM4ufmnPfdR
BuQWzAC5FbgA1sCkRk3+D/CyO0H9k5PP
...