✅ fanquake closed an issue: "integer overflow in FUZZ=package_rbf"
(https://github.com/bitcoin/bitcoin/issues/33981)
(https://github.com/bitcoin/bitcoin/issues/33981)
🚀 fanquake merged a pull request: "fuzz: gate mempool entry based on weight"
(https://github.com/bitcoin/bitcoin/pull/33985)
(https://github.com/bitcoin/bitcoin/pull/33985)
💬 Sjors commented on issue "Mining IPC `createNewBlock` should not return before IBD is over":
(https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602538081)
Makes sense. We'll have to find another method of preventing a burst of new templates while the node churns through the last day worth of blocks.
The Template Provider client could simply wait 1 minute at startup if IBD was initially true and changed to false. In practice that only happens after the _node_ restarts (we wouldn't want to waste one minute each time the TP restarts).
(https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602538081)
Makes sense. We'll have to find another method of preventing a burst of new templates while the node churns through the last day worth of blocks.
The Template Provider client could simply wait 1 minute at startup if IBD was initially true and changed to false. In practice that only happens after the _node_ restarts (we wouldn't want to waste one minute each time the TP restarts).
💬 plebhash commented on issue "Mining IPC `createNewBlock` should not return before IBD is over":
(https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602551596)
> Can you please provide the logs and setups accessible in a text file and not a discord link I think some people might not have discord.
yeah sorry I was in a hurry for the workshop, indeed pasting Discord links is not ideal
this is what @jbesraa said:
> I think handling IBD and other initialization aspects is critical when working in production env.
> As these two services need to set on the same machine you usually deploy them through the same "task" and I think they should wait for each o
...
(https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602551596)
> Can you please provide the logs and setups accessible in a text file and not a discord link I think some people might not have discord.
yeah sorry I was in a hurry for the workshop, indeed pasting Discord links is not ideal
this is what @jbesraa said:
> I think handling IBD and other initialization aspects is critical when working in production env.
> As these two services need to set on the same machine you usually deploy them through the same "task" and I think they should wait for each o
...
💬 roconnor-blockstream commented on issue "Standardness policy rules for legacy Multisig script is incoherent":
(https://github.com/bitcoin/bitcoin/issues/33882#issuecomment-3602626488)
> I think I disagree with this -- we allow spends of fairly arbitrary p2sh scripts, but limit legacy spends to only a few known templates. That's a good thing because it reserves a lot of space for upgrade hooks, which have been used for p2sh, segwit and taproot.
One thing I suggested to @151henry151 was to not only limit the number of checksig operations but to require at least one checksig operation. Does that make you more comfortable?
(https://github.com/bitcoin/bitcoin/issues/33882#issuecomment-3602626488)
> I think I disagree with this -- we allow spends of fairly arbitrary p2sh scripts, but limit legacy spends to only a few known templates. That's a good thing because it reserves a lot of space for upgrade hooks, which have been used for p2sh, segwit and taproot.
One thing I suggested to @151henry151 was to not only limit the number of checksig operations but to require at least one checksig operation. Does that make you more comfortable?
💬 fjahr commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-3602628277)
re-ACK cb7d5bfe4a59d6034e3a77486cbd423e1a3bfa44
Confirmed the changes were addressing comments and a rebase.
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-3602628277)
re-ACK cb7d5bfe4a59d6034e3a77486cbd423e1a3bfa44
Confirmed the changes were addressing comments and a rebase.
💬 plebhash commented on issue "Mining IPC `createNewBlock` should not return before IBD is over":
(https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602646676)
> `IsInitialBlockDownload()` is essentially this very conservative check. Only if you're really behind it'll say true.
how far behind? are we talking ~2, ~20 or ~200 blocks behind?
(https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602646676)
> `IsInitialBlockDownload()` is essentially this very conservative check. Only if you're really behind it'll say true.
how far behind? are we talking ~2, ~20 or ~200 blocks behind?
💬 sipa commented on issue "Mining IPC `createNewBlock` should not return before IBD is over":
(https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602654786)
> how far behind? are we talking ~2, ~20 or ~200 blocks behind?
A day.
(https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602654786)
> how far behind? are we talking ~2, ~20 or ~200 blocks behind?
A day.
👍 ryanofsky approved a pull request: "mining: add getCoinbase()"
(https://github.com/bitcoin/bitcoin/pull/33819#pullrequestreview-3527399289)
Code review ACK 73553c809ef6517839c4d1b02b657e6fa401f8a8. I left some minor suggestions below but they are not important. Thanks for responding to all the previous ones.
AJ's bigger suggestions https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3539952304 and https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3545336207 definitely make a lot of sense to me, and would seem be a simpler & more future-proof direction to go in. They seem directionally compatible with this change, th
...
(https://github.com/bitcoin/bitcoin/pull/33819#pullrequestreview-3527399289)
Code review ACK 73553c809ef6517839c4d1b02b657e6fa401f8a8. I left some minor suggestions below but they are not important. Thanks for responding to all the previous ones.
AJ's bigger suggestions https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3539952304 and https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3545336207 definitely make a lot of sense to me, and would seem be a simpler & more future-proof direction to go in. They seem directionally compatible with this change, th
...
💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2581603580)
In commit "mining: add getCoinbase()" (73553c809ef6517839c4d1b02b657e6fa401f8a8)
Not important, but it would be nice if fields were extracted in same order they are declared: version, sequence, script_sig, etc.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2581603580)
In commit "mining: add getCoinbase()" (73553c809ef6517839c4d1b02b657e6fa401f8a8)
Not important, but it would be nice if fields were extracted in same order they are declared: version, sequence, script_sig, etc.
💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2581576868)
In commit "mining: add getCoinbase()" (73553c809ef6517839c4d1b02b657e6fa401f8a8)
Was it intentional not to include the "note deprecated: use getCoinbase()" comment on this method? Would be good to include if not.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2581576868)
In commit "mining: add getCoinbase()" (73553c809ef6517839c4d1b02b657e6fa401f8a8)
Was it intentional not to include the "note deprecated: use getCoinbase()" comment on this method? Would be good to include if not.
💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2579000680)
In commit "mining: add getCoinbase()" (73553c809ef6517839c4d1b02b657e6fa401f8a8)
Instead of having a warning here it would seem better to document the compatibility concern directly in the code generating the scriptsig. This way, a potentially breaking change would be obvious just looking at this code and not rely on noticing a warning elsewhere. Also it would seem more future-proof for IPC documentation to just say that the final scriptsig needs to be <= 100 bytes, instead of trying to guara
...
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2579000680)
In commit "mining: add getCoinbase()" (73553c809ef6517839c4d1b02b657e6fa401f8a8)
Instead of having a warning here it would seem better to document the compatibility concern directly in the code generating the scriptsig. This way, a potentially breaking change would be obvious just looking at this code and not rely on noticing a warning elsewhere. Also it would seem more future-proof for IPC documentation to just say that the final scriptsig needs to be <= 100 bytes, instead of trying to guara
...
💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2581511244)
re: https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514301870
> As a progression towards `getCoinbase` & deprecation, this implementation could internally also do what is suggested in the deprecation node: invoke `ExtractCoinbaseTemplate`, scan outputs for the SegWit marker, and return the index.
I don't see a clean way to get the index from ExtractCoinbaseTemplate, but even if there were, calling `GetWitnessCommitmentIndex` seems like the simplest implementation of this method.
...
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2581511244)
re: https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514301870
> As a progression towards `getCoinbase` & deprecation, this implementation could internally also do what is suggested in the deprecation node: invoke `ExtractCoinbaseTemplate`, scan outputs for the SegWit marker, and return the index.
I don't see a clean way to get the index from ExtractCoinbaseTemplate, but even if there were, calling `GetWitnessCommitmentIndex` seems like the simplest implementation of this method.
...
💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2581564930)
re: https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533740877
> I added documentation there, but we also need it here. The `output` field is not limited to `OP_RETURN`, that's just a heuristic used by `ExtractCoinbaseTemplate`.
Seem like with latest update, this comment is no longer accurate and should be removed? I guess still I don't understand why it would be important to document the outputs field in this function declaration instead of only in the struct definition, but may
...
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2581564930)
re: https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533740877
> I added documentation there, but we also need it here. The `output` field is not limited to `OP_RETURN`, that's just a heuristic used by `ExtractCoinbaseTemplate`.
Seem like with latest update, this comment is no longer accurate and should be removed? I guess still I don't understand why it would be important to document the outputs field in this function declaration instead of only in the struct definition, but may
...
💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2579049804)
re: https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533690075
> The BIP says:
>
> > For backward compatibility, the `Hash(new commitment|witness reserved value)` will go to the coinbase witness, and the witness reserved value will be recorded in another location specified by the future softfork. Any number of new commitment could be added in this way.
>
> So we provide clients with the coinbase witness, which is not necessarily the "witness reserved value".
Wow, I see. S
...
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2579049804)
re: https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533690075
> The BIP says:
>
> > For backward compatibility, the `Hash(new commitment|witness reserved value)` will go to the coinbase witness, and the witness reserved value will be recorded in another location specified by the future softfork. Any number of new commitment could be added in this way.
>
> So we provide clients with the coinbase witness, which is not necessarily the "witness reserved value".
Wow, I see. S
...
💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2581390462)
re: https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533772625
Thanks for spelling out the hypothetical here. The new comments are also nice and make intent more clear.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2581390462)
re: https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533772625
Thanks for spelling out the hypothetical here. The new comments are also nice and make intent more clear.
💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2579073847)
re: https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533690075
> IMO `witness_reserved_value` would be a more descriptive name than just `witness`.
Other response https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533690075 why this name would be inconsistent with terminology of BIP141.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2579073847)
re: https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533690075
> IMO `witness_reserved_value` would be a more descriptive name than just `witness`.
Other response https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533690075 why this name would be inconsistent with terminology of BIP141.
💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2581294471)
re: https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533732177
> That seems incorrect, because a pool (like Ocean or P2pool) might let you take part of the reward directly in an output.
Hmm, it sounds like the idea here is maybe that pools will reuse this `CoinbaseTemplate` struct, and prepend their own outputs into `required_outputs` and subtract their own rewards from `value_remaining` leaving the individual miner to take the rest? I'm not sure how realistic it is to expect poo
...
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2581294471)
re: https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533732177
> That seems incorrect, because a pool (like Ocean or P2pool) might let you take part of the reward directly in an output.
Hmm, it sounds like the idea here is maybe that pools will reuse this `CoinbaseTemplate` struct, and prepend their own outputs into `required_outputs` and subtract their own rewards from `value_remaining` leaving the individual miner to take the rest? I'm not sure how realistic it is to expect poo
...
📝 fanquake opened a pull request: "contrib: fix manpage generation"
(https://github.com/bitcoin/bitcoin/pull/33996)
0972f5504021b482b27523fd3bcb8036cf6b439c from #33229 broke manpage generation, because the assumption that the last word in the line containing the version number, was the version number, no-longer holds for some binaries. i.e `bitcoind`.
(https://github.com/bitcoin/bitcoin/pull/33996)
0972f5504021b482b27523fd3bcb8036cf6b439c from #33229 broke manpage generation, because the assumption that the last word in the line containing the version number, was the version number, no-longer holds for some binaries. i.e `bitcoind`.
💬 fanquake commented on pull request "multiprocess: Don't require bitcoin -m argument when IPC options are used":
(https://github.com/bitcoin/bitcoin/pull/33229#issuecomment-3602751889)
Fixed in #33996.
(https://github.com/bitcoin/bitcoin/pull/33229#issuecomment-3602751889)
Fixed in #33996.