💬 fanquake commented on pull request "fuzz: gate mempool entry based on weight":
(https://github.com/bitcoin/bitcoin/pull/33985#issuecomment-3602185685)
cc @dergoegge
(https://github.com/bitcoin/bitcoin/pull/33985#issuecomment-3602185685)
cc @dergoegge
💬 Sjors commented on pull request "refactor: disentangle miner startup defaults from runtime options":
(https://github.com/bitcoin/bitcoin/pull/33966#issuecomment-3602275695)
Rebased on the latest #33965 which allowed for some simplifications:
- `ClampOptions` no longer takes a `MiningArgs` argument
- no modifications to RPC code
- the new `block_max_weight` option on `BlockCreateOptions` will no longer be ignored if a caller sets it, only the tx_pool fuzzer does.
We could trivially make the new `block_max_weight` option available to IPC clients, but I don't think we should encourage its use.
Note that I brought back `ApplyArgsManOptions` in `miner.cpp`, but
...
(https://github.com/bitcoin/bitcoin/pull/33966#issuecomment-3602275695)
Rebased on the latest #33965 which allowed for some simplifications:
- `ClampOptions` no longer takes a `MiningArgs` argument
- no modifications to RPC code
- the new `block_max_weight` option on `BlockCreateOptions` will no longer be ignored if a caller sets it, only the tx_pool fuzzer does.
We could trivially make the new `block_max_weight` option available to IPC clients, but I don't think we should encourage its use.
Note that I brought back `ApplyArgsManOptions` in `miner.cpp`, but
...
💬 Sjors commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#issuecomment-3602323654)
Rebased after #33591 because #33966 has a trivial merge conflict with it and I want to keep that cleanly based on this PR.
(https://github.com/bitcoin/bitcoin/pull/33965#issuecomment-3602323654)
Rebased after #33591 because #33966 has a trivial merge conflict with it and I want to keep that cleanly based on this PR.
💬 ajtowns commented on issue "Standardness policy rules for legacy Multisig script is incoherent":
(https://github.com/bitcoin/bitcoin/issues/33882#issuecomment-3602342513)
> P2SH allows non-solvable redeem scripts with low sigop counts (PR https://github.com/bitcoin/bitcoin/pull/4365), while legacy scripts still require solvability. This inconsistency is hard to justify, especially since both rely on sigop limits for security.
> I agree. I think my next step would be to PR a change to `AreInputsStandard` to bring P2SH and legacy script policy into alignment.
I think I disagree with this -- we allow spends of fairly arbitrary p2sh scripts, but limit legacy spends
...
(https://github.com/bitcoin/bitcoin/issues/33882#issuecomment-3602342513)
> P2SH allows non-solvable redeem scripts with low sigop counts (PR https://github.com/bitcoin/bitcoin/pull/4365), while legacy scripts still require solvability. This inconsistency is hard to justify, especially since both rely on sigop limits for security.
> I agree. I think my next step would be to PR a change to `AreInputsStandard` to bring P2SH and legacy script policy into alignment.
I think I disagree with this -- we allow spends of fairly arbitrary p2sh scripts, but limit legacy spends
...
💬 Sjors commented on issue "Mining IPC `createNewBlock` should not return before IBD is over":
(https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602376905)
The problem is that `ChainstateManager::IsInitialBlockDownload()` uses a fairly relaxed heuristic:
https://github.com/bitcoin/bitcoin/blob/d0f6d9953a15d7c7111d46dcb76ab2bb18e5dee3/src/validation.cpp#L1998-L2023
We could make this function much more precise by having it consider the most proof-of-work (not invalid) header chain.
But that might have impacts in other places in the codebase, potentially causing things to get stuck longer than needed just because we're missing a few recent blocks.
...
(https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602376905)
The problem is that `ChainstateManager::IsInitialBlockDownload()` uses a fairly relaxed heuristic:
https://github.com/bitcoin/bitcoin/blob/d0f6d9953a15d7c7111d46dcb76ab2bb18e5dee3/src/validation.cpp#L1998-L2023
We could make this function much more precise by having it consider the most proof-of-work (not invalid) header chain.
But that might have impacts in other places in the codebase, potentially causing things to get stuck longer than needed just because we're missing a few recent blocks.
...
💬 sipa commented on issue "Mining IPC `createNewBlock` should not return before IBD is over":
(https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602401780)
You can't use "active_tip == best_known_block_header" as condition for mining, because an attacker could mine a block, relay the header, and never relay the block. Sure, very expensive, but if the outcome is shutting down other miners, some joker might try it.
Bitcoin nodes are *always* synchronizing, and unless you have very strong evidence to the contrary, you should act as if your active tip is the best block on the chain. If we had evidence of a better block, it would be our best tip instea
...
(https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602401780)
You can't use "active_tip == best_known_block_header" as condition for mining, because an attacker could mine a block, relay the header, and never relay the block. Sure, very expensive, but if the outcome is shutting down other miners, some joker might try it.
Bitcoin nodes are *always* synchronizing, and unless you have very strong evidence to the contrary, you should act as if your active tip is the best block on the chain. If we had evidence of a better block, it would be our best tip instea
...
👍 ismaelsadeeq approved a pull request: "fuzz: gate mempool entry based on weight"
(https://github.com/bitcoin/bitcoin/pull/33985#pullrequestreview-3530516004)
utACK 804329400a73df00dfd7a5209c659d4a22b9ce47
(https://github.com/bitcoin/bitcoin/pull/33985#pullrequestreview-3530516004)
utACK 804329400a73df00dfd7a5209c659d4a22b9ce47
📝 hebasto opened a pull request: "depends: Propagate native C compiler to `sqlite` package"
(https://github.com/bitcoin/bitcoin/pull/33995)
This PR:
1. Ensures that autosetup can build the local bootstrap `jimsh0` when neither `jimsh` nor `tclsh` is available on the system.
2. Removes the `tcl` package from the Guix manifest.
This is an alternative to https://github.com/bitcoin/bitcoin/pull/33975.
(https://github.com/bitcoin/bitcoin/pull/33995)
This PR:
1. Ensures that autosetup can build the local bootstrap `jimsh0` when neither `jimsh` nor `tclsh` is available on the system.
2. Removes the `tcl` package from the Guix manifest.
This is an alternative to https://github.com/bitcoin/bitcoin/pull/33975.
💬 hebasto commented on pull request "depends, doc: Add `tcl` as build dependency for `sqlite` package":
(https://github.com/bitcoin/bitcoin/pull/33975#issuecomment-3602441211)
> Can we just pass the compiler through?
See https://github.com/bitcoin/bitcoin/pull/33995.
(https://github.com/bitcoin/bitcoin/pull/33975#issuecomment-3602441211)
> Can we just pass the compiler through?
See https://github.com/bitcoin/bitcoin/pull/33995.
👍 dergoegge approved a pull request: "fuzz: gate mempool entry based on weight"
(https://github.com/bitcoin/bitcoin/pull/33985#pullrequestreview-3530571242)
utACK 804329400a73df00dfd7a5209c659d4a22b9ce47
(https://github.com/bitcoin/bitcoin/pull/33985#pullrequestreview-3530571242)
utACK 804329400a73df00dfd7a5209c659d4a22b9ce47
✅ 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.