💬 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
...
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2479440054)
Yes, we do. The reason is explained there. All thread busy, a new task arrives so notifications goes unnoticed, then threads finish processing and sleep. Loosing the new task wake up notification, not running the new task and sleeping until a new one gets submitted. There is a test exercising this exact behavior.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2479440054)
Yes, we do. The reason is explained there. All thread busy, a new task arrives so notifications goes unnoticed, then threads finish processing and sleep. Loosing the new task wake up notification, not running the new task and sleeping until a new one gets submitted. There is a test exercising this exact behavior.
💬 brunoerg commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3470018219)
I was reading the code without having seen the discussions here, only based on PR title and description. With some context from the discussions, I think both of them should be updated to reflect what this PR is really doing. It was asked before (see https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3353039818) but maybe it still needs improvement.
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3470018219)
I was reading the code without having seen the discussions here, only based on PR title and description. With some context from the discussions, I think both of them should be updated to reflect what this PR is really doing. It was asked before (see https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3353039818) but maybe it still needs improvement.
💬 davidgumberg commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3470028981)
We actually add peers we send `GETDATA`'s to in response to headers to `mapBlocksInFlight`[^1], so in the scenario you describe we would treat the peer that announced the header to us as `first_in_flight()`.
The flow is [`ProcessHeadersMessage()`](https://github.com/bitcoin/bitcoin/blob/6f359695c36c13af832b0b1021dcb3a234f6f219/src/net_processing.cpp#L2829-L2831), and if the header looks good, call [`HeadersDirectFetchBlocks()`](https://github.com/bitcoin/bitcoin/blob/6f359695c36c13af832b0b10
...
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3470028981)
We actually add peers we send `GETDATA`'s to in response to headers to `mapBlocksInFlight`[^1], so in the scenario you describe we would treat the peer that announced the header to us as `first_in_flight()`.
The flow is [`ProcessHeadersMessage()`](https://github.com/bitcoin/bitcoin/blob/6f359695c36c13af832b0b1021dcb3a234f6f219/src/net_processing.cpp#L2829-L2831), and if the header looks good, call [`HeadersDirectFetchBlocks()`](https://github.com/bitcoin/bitcoin/blob/6f359695c36c13af832b0b10
...
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2479467969)
That is part of the possible future experimentations written in the PR description. Goal is to keep the same synchronization mechanisms we had in the http server code.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2479467969)
That is part of the possible future experimentations written in the PR description. Goal is to keep the same synchronization mechanisms we had in the http server code.
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2479480850)
`num_tasks` is an int. That would require casting it to size_t.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2479480850)
`num_tasks` is an int. That would require casting it to size_t.
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2479504434)
Thanks - added in [301116e855...c30647c4d3](https://github.com/bitcoin/bitcoin/compare/301116e85540e49c27473f450fc210b702db4cf5..c30647c4d34c2941696729704854467b30657c43).
Also, tested an invalid RFC 3986 query:
```
$ curl -v 'http://localhost:8332/rest/blockpart/000000000000000000016d31a675b96acb4aacca6e4653039703b9fe03997c78.hex?%XY'
* Host localhost:8332 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
* Trying [::1]:8332...
* Connected to localhost (::1) port 8332
> GET /rest/blockpa
...
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2479504434)
Thanks - added in [301116e855...c30647c4d3](https://github.com/bitcoin/bitcoin/compare/301116e85540e49c27473f450fc210b702db4cf5..c30647c4d34c2941696729704854467b30657c43).
Also, tested an invalid RFC 3986 query:
```
$ curl -v 'http://localhost:8332/rest/blockpart/000000000000000000016d31a675b96acb4aacca6e4653039703b9fe03997c78.hex?%XY'
* Host localhost:8332 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
* Trying [::1]:8332...
* Connected to localhost (::1) port 8332
> GET /rest/blockpa
...
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2479509680)
This decision was on purpose. To be able to assert and abort the program if a negative number is provided. A negative integer to size_t conversion would be bad.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2479509680)
This decision was on purpose. To be able to assert and abort the program if a negative number is provided. A negative integer to size_t conversion would be bad.
✅ roodmandev-ux closed an issue: "1"
(https://github.com/bitcoin/bitcoin/issues/33748)
(https://github.com/bitcoin/bitcoin/issues/33748)
💬 roodmandev-ux commented on issue "1":
(https://github.com/bitcoin/bitcoin/issues/33748#issuecomment-3470300182)
-
(https://github.com/bitcoin/bitcoin/issues/33748#issuecomment-3470300182)
-
💬 TheCharlatan commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479587224)
In commit 504acfcd5d1ed4d77f7fd4f5dc57e66d6718d3d4:
If I apply this test independently before the miner.cpp code changes, it still passes, so not sure what we are testing here. Wouldn't this only be relevant if we'd run checkBlock on the template's own block? Afaict we only do checkBlock on blocks we re-create here.
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479587224)
In commit 504acfcd5d1ed4d77f7fd4f5dc57e66d6718d3d4:
If I apply this test independently before the miner.cpp code changes, it still passes, so not sure what we are testing here. Wouldn't this only be relevant if we'd run checkBlock on the template's own block? Afaict we only do checkBlock on blocks we re-create here.