✅ ismaelsadeeq closed a pull request: "fees: rpc: `estimatesmartfee` now returns a fee rate estimate during low network activity"
(https://github.com/bitcoin/bitcoin/pull/32395)
(https://github.com/bitcoin/bitcoin/pull/32395)
💬 ismaelsadeeq commented on pull request "fees: rpc: `estimatesmartfee` now returns a fee rate estimate during low network activity":
(https://github.com/bitcoin/bitcoin/pull/32395#issuecomment-3469165154)
Closing this due to lack of interest.
(https://github.com/bitcoin/bitcoin/pull/32395#issuecomment-3469165154)
Closing this due to lack of interest.
💬 ismaelsadeeq commented on issue "Add a script for Signet or Regtest to unlock fee estimator":
(https://github.com/bitcoin/bitcoin/issues/32105#issuecomment-3469218336)
Reviewers of #32395 can also share weigh input here so we don’t keep this open unnecessarily.
What @yellowred suggested can be handled by the client that actually needs this functionality, rather than us having to write and maintain this script in our repository.
I took a pragmatic approach, but it received little to mixed support (~0 and +1), so it seems the general sentiment is that we don’t want to enable this. Based on the feedback from my previous (now closed) PR, this can be closed as w
...
(https://github.com/bitcoin/bitcoin/issues/32105#issuecomment-3469218336)
Reviewers of #32395 can also share weigh input here so we don’t keep this open unnecessarily.
What @yellowred suggested can be handled by the client that actually needs this functionality, rather than us having to write and maintain this script in our repository.
I took a pragmatic approach, but it received little to mixed support (~0 and +1), so it seems the general sentiment is that we don’t want to enable this. Based on the feedback from my previous (now closed) PR, this can be closed as w
...
💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2478967921)
I took a stab at it and it's a bit tedious (I managed to move it up earlier in the commit history, but stopped when I got to a conflict with a commit that also touches truc_policy in different ways).
My overall take is it's not such a big deal, as the concern is not around correctness of the code but a potential CPU DoS if exposed over the network, which doesn't strike me as all that concerning for an intermediate commit.
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2478967921)
I took a stab at it and it's a bit tedious (I managed to move it up earlier in the commit history, but stopped when I got to a conflict with a commit that also touches truc_policy in different ways).
My overall take is it's not such a big deal, as the concern is not around correctness of the code but a potential CPU DoS if exposed over the network, which doesn't strike me as all that concerning for an intermediate commit.
💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#issuecomment-3469378663)
Latest push ed813c48f826d083becf93c741b483774c850c86 -> f2ce362:
- implements `GetStrongRandBytes` per [comment](https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2410849029)
- modifies `create_tx` to choose a mempool UTXO only sometimes instead of most of the time
- modifies `create_block` to generate more than 2 non-coinbase transactions per feedback from review club
(https://github.com/bitcoin/bitcoin/pull/33300#issuecomment-3469378663)
Latest push ed813c48f826d083becf93c741b483774c850c86 -> f2ce362:
- implements `GetStrongRandBytes` per [comment](https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2410849029)
- modifies `create_tx` to choose a mempool UTXO only sometimes instead of most of the time
- modifies `create_block` to generate more than 2 non-coinbase transactions per feedback from review club
📝 hulxv opened a pull request: "feat: shell completions for bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/33747)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/33747)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
💬 frankomosh commented on pull request "doc: reference fuzz coverage steps in quick-start":
(https://github.com/bitcoin/bitcoin/pull/33536#issuecomment-3469389865)
> @frankomosh the PR description needs updating to reflect the current change?
>
> cc @dergoegge @marcofleon
yeah indeed
(https://github.com/bitcoin/bitcoin/pull/33536#issuecomment-3469389865)
> @frankomosh the PR description needs updating to reflect the current change?
>
> cc @dergoegge @marcofleon
yeah indeed
🤔 fjahr reviewed a pull request: "RPC/txoutproof: Support including (and verifying) proofs of wtxid"
(https://github.com/bitcoin/bitcoin/pull/32844#pullrequestreview-3400931461)
Concept ACK
Is there something special about the naming "generation tx" here? It seems to be just an older alias for the coinbase transaction without any other special meaning. I can not recall seeing it used anywhere in the code base or online discourse. If I am not overlooking something it would probably be better to just call it coinbase tx instead which should be less confusing to newer contributors that haven't heard this term before.
(https://github.com/bitcoin/bitcoin/pull/32844#pullrequestreview-3400931461)
Concept ACK
Is there something special about the naming "generation tx" here? It seems to be just an older alias for the coinbase transaction without any other special meaning. I can not recall seeing it used anywhere in the code base or online discourse. If I am not overlooking something it would probably be better to just call it coinbase tx instead which should be less confusing to newer contributors that haven't heard this term before.
💬 fjahr commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2479023056)
Should also cover an invalid proof here
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2479023056)
Should also cover an invalid proof here
💬 fjahr commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2479054592)
I think that makes sense and we could even do the same for `gettxoutproof` I think (if that wasn't already intended with the comment as well).
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2479054592)
I think that makes sense and we could even do the same for `gettxoutproof` I think (if that wasn't already intended with the comment as well).
💬 davidgumberg commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479106793)
The comment here could at least be changed to something like:
```suggestion
* @returns if the block was processed, does not necessarily indicate validity.
```
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479106793)
The comment here could at least be changed to something like:
```suggestion
* @returns if the block was processed, does not necessarily indicate validity.
```
💬 pinheadmz commented on issue "Add a script for Signet or Regtest to unlock fee estimator":
(https://github.com/bitcoin/bitcoin/issues/32105#issuecomment-3469611314)
Agree (to close the issue), I have several scripts like this for different use cases anyway. Thanks for putting in the work! This question will definitely come up again and the closed PR can justify a "wont fix"
(https://github.com/bitcoin/bitcoin/issues/32105#issuecomment-3469611314)
Agree (to close the issue), I have several scripts like this for different use cases anyway. Thanks for putting in the work! This question will definitely come up again and the closed PR can justify a "wont fix"
💬 marcofleon commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#issuecomment-3469642268)
I've been playing around with the fuzz test and wanted to update here, even if it's still a bit inconclusive.
I'm running into non-reproducible timeouts when using `fork` with libFuzzer. I'm overusing my cores by a lot (`fork=25` plus the 3 workers each), and I'm not yet sure if these timeouts would happen eventually when using less cores.
I have "fixed" this by changing `notify_one` in `Submit` to `notify_all`, although I can't say I'm exactly sure why this works. It almost seems like so
...
(https://github.com/bitcoin/bitcoin/pull/33689#issuecomment-3469642268)
I've been playing around with the fuzz test and wanted to update here, even if it's still a bit inconclusive.
I'm running into non-reproducible timeouts when using `fork` with libFuzzer. I'm overusing my cores by a lot (`fork=25` plus the 3 workers each), and I'm not yet sure if these timeouts would happen eventually when using less cores.
I have "fixed" this by changing `notify_one` in `Submit` to `notify_all`, although I can't say I'm exactly sure why this works. It almost seems like so
...
💬 ajtowns commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3469806048)
> The suggested diff doesn't work because we remove the block request from mapBlocksInFlight if the txn's are missing, the firstInFlight slot is already taken, and the block is not from an HB Peer:
Does that logic make sense? If we receive:
```mermaid
sequenceDiagram
participant A
Us ->> A: SENDCMPCT 0
Us ->> B: SENDCMPCT 1 (high-bandwidth)
A ->> Us: t=0 HEADERS
Us ->> A: GETDATA(CMPCT)
B ->> Us: t=25ms CMPCTBLOCK
Us ->> B: GETBLOCKTXN
A ->> Us: t=50ms CMPC
...
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3469806048)
> The suggested diff doesn't work because we remove the block request from mapBlocksInFlight if the txn's are missing, the firstInFlight slot is already taken, and the block is not from an HB Peer:
Does that logic make sense? If we receive:
```mermaid
sequenceDiagram
participant A
Us ->> A: SENDCMPCT 0
Us ->> B: SENDCMPCT 1 (high-bandwidth)
A ->> Us: t=0 HEADERS
Us ->> A: GETDATA(CMPCT)
B ->> Us: t=25ms CMPCTBLOCK
Us ->> B: GETBLOCKTXN
A ->> Us: t=50ms CMPC
...
💬 polespinasa commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2479409437)
You are right, although I think it would not be a problem because we don't print that many decimals.
Anyway, I think the approach in afeeac1596da47b0e431d468a665d344b39e1a87 is cleaner and follows what we already do for btc/kvB. What do you think?
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2479409437)
You are right, although I think it would not be a problem because we don't print that many decimals.
Anyway, I think the approach in afeeac1596da47b0e431d468a665d344b39e1a87 is cleaner and follows what we already do for btc/kvB. What do you think?
💬 polespinasa commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2479414797)
I think with the rebase it's easier to change defaults without touching code logic.
What would be the benefits of using `self.MaybeArg<bool>`? I'm not really familiar with it tbh.
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2479414797)
I think with the rebase it's easier to change defaults without touching code logic.
What would be the benefits of using `self.MaybeArg<bool>`? I'm not really familiar with it tbh.
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2479418360)
It is a known issue. The pcp fuzz test does it too for the same reason (in an undocumented manner). I only document it properly so we don't forget this exists. Feel free to investigate it in a separate issue/PR. I'm not planning to do it. It is not an issue of the code introduced in this PR.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2479418360)
It is a known issue. The pcp fuzz test does it too for the same reason (in an undocumented manner). I only document it properly so we don't forget this exists. Feel free to investigate it in a separate issue/PR. I'm not planning to do it. It is not an issue of the code introduced in this PR.
💬 brunoerg commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2479421965)
aeb18a134620c61c4518a357fea651e7c8d1a024: This could be simplified. These strings ("Send ... BTC") are not being used.
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2479421965)
aeb18a134620c61c4518a357fea651e7c8d1a024: This could be simplified. These strings ("Send ... BTC") are not being used.
💬 brunoerg commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2479434096)
aeb18a134620c61c4518a357fea651e7c8d1a024: This `assert_equal` seems unnecessary. You're basically doing:
```py
a = b
assert a == b
```
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2479434096)
aeb18a134620c61c4518a357fea651e7c8d1a024: This `assert_equal` seems unnecessary. You're basically doing:
```py
a = b
assert a == b
```
💬 polespinasa commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#issuecomment-3469979496)
@maflcko thanks for reviewing :)
> I guess it is a bit verbose to have each RPC annotated with a type optional arg, but there probably isn't an easier solution.
I don't think there is either, at least not in a backwards compatible way.
> There could be a global option to toggle the default, to improve the UX.
That would be great for a `bitcoin-cli` user, but probably fatal for other programs that rely on the Bitcoin Core RPC.
E.g. if a user with a Lightning Node enables sat/vB, thu
...
(https://github.com/bitcoin/bitcoin/pull/33741#issuecomment-3469979496)
@maflcko thanks for reviewing :)
> I guess it is a bit verbose to have each RPC annotated with a type optional arg, but there probably isn't an easier solution.
I don't think there is either, at least not in a backwards compatible way.
> There could be a global option to toggle the default, to improve the UX.
That would be great for a `bitcoin-cli` user, but probably fatal for other programs that rely on the Bitcoin Core RPC.
E.g. if a user with a Lightning Node enables sat/vB, thu
...