💬 jonatack commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1944998630)
Thanks for the reminder, for some reason I keep thinking i.e. means "for example."
Are you suggesting s/i.e./e.g./, or removing the example? My thought was that this doc is a bit more user-facing, so more explanation or an example might be helpful.
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1944998630)
Thanks for the reminder, for some reason I keep thinking i.e. means "for example."
Are you suggesting s/i.e./e.g./, or removing the example? My thought was that this doc is a bit more user-facing, so more explanation or an example might be helpful.
💬 ismaelsadeeq commented on issue "Fee Estimation via Fee rate Forecasters tracking issue":
(https://github.com/bitcoin/bitcoin/issues/30392#issuecomment-2640268166)
Hmm, I am not sure whether this is a rational fee rate.
1. It's also unclear what confirmation target this "rational fee rate" is aiming for.
2. I see there is a case where you just 1000 blindly this is not rational at all, it may be that your mempool is sparse because you just started your node, and you only have 1 block worth of transactions.
3. The script you provided does not also check against the minimum relay fee rate. Hence you might provide a fee rate below that and your transaction
...
(https://github.com/bitcoin/bitcoin/issues/30392#issuecomment-2640268166)
Hmm, I am not sure whether this is a rational fee rate.
1. It's also unclear what confirmation target this "rational fee rate" is aiming for.
2. I see there is a case where you just 1000 blindly this is not rational at all, it may be that your mempool is sparse because you just started your node, and you only have 1 block worth of transactions.
3. The script you provided does not also check against the minimum relay fee rate. Hence you might provide a fee rate below that and your transaction
...
💬 ajtowns commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2640271485)
> > Running with -reindex-chainstate will leave you stuck in an infinite loop revalidating an invalid tip?
>
> I wouldn't expect that. `ConnectTip()` will call `InvalidBlockFound()` which should mark the block index as failed, and we won't revalidate / instead will stay at the last valid block - same scenario as if it failed the script checks.
>
> I can see the infinite loop happening only if a previously admitted block fails with `BLOCK_MUTATED` during connection, but mutation rules shoul
...
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2640271485)
> > Running with -reindex-chainstate will leave you stuck in an infinite loop revalidating an invalid tip?
>
> I wouldn't expect that. `ConnectTip()` will call `InvalidBlockFound()` which should mark the block index as failed, and we won't revalidate / instead will stay at the last valid block - same scenario as if it failed the script checks.
>
> I can see the infinite loop happening only if a previously admitted block fails with `BLOCK_MUTATED` during connection, but mutation rules shoul
...
💬 instagibbs commented on pull request "test: fixes p2p_ibd_txrelay wait time":
(https://github.com/bitcoin/bitcoin/pull/31759#issuecomment-2640284141)
@glozow managed to misread the changeset, nevermind
(https://github.com/bitcoin/bitcoin/pull/31759#issuecomment-2640284141)
@glozow managed to misread the changeset, nevermind
💬 naiyoma commented on issue "rpc: show P2(W)SH redeemScript in getrawtransaction (and friends)":
(https://github.com/bitcoin/bitcoin/issues/27391#issuecomment-2640292027)
@krishpranav there's an open pr for this here -> https://github.com/bitcoin/bitcoin/pull/31252
(https://github.com/bitcoin/bitcoin/issues/27391#issuecomment-2640292027)
@krishpranav there's an open pr for this here -> https://github.com/bitcoin/bitcoin/pull/31252
💬 jonatack commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1945028593)
Removed the example here.
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1945028593)
Removed the example here.
💬 jonatack commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1945036311)
Shortened the line length in the last push.
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1945036311)
Shortened the line length in the last push.
💬 sipa commented on issue "nSequence is not set when spending from satisfiable descriptor with relative timelock":
(https://github.com/bitcoin/bitcoin/issues/31808#issuecomment-2640347090)
We discussed this a bit off-line.
There are a number of issues here, and the simple ones can be solved in specialized cases, but the general problem is really a sign of a deeper issue.
The superficial problems are at least these, which stem from the specific order decisions are made: (1) coin selection, (2) transaction building, (3) input signing.
* Coin selection may be picking coins which are not (yet) spendable (assuming the intent is to spend right now, and not something to be presigned an
...
(https://github.com/bitcoin/bitcoin/issues/31808#issuecomment-2640347090)
We discussed this a bit off-line.
There are a number of issues here, and the simple ones can be solved in specialized cases, but the general problem is really a sign of a deeper issue.
The superficial problems are at least these, which stem from the specific order decisions are made: (1) coin selection, (2) transaction building, (3) input signing.
* Coin selection may be picking coins which are not (yet) spendable (assuming the intent is to spend right now, and not something to be presigned an
...
📝 instagibbs opened a pull request: "test: test_inv_block, use mocktime instead of waiting"
(https://github.com/bitcoin/bitcoin/pull/31811)
Performance issue reported in https://github.com/bitcoin/bitcoin/pull/31437#issuecomment-2640221382
It seems that code as-is waits for wall-clock time to pass to synchronize mempools. Locally, sometimes the subtest takes a couple seconds, sometimes it takes an additional minute.
Just use mocktime?
(https://github.com/bitcoin/bitcoin/pull/31811)
Performance issue reported in https://github.com/bitcoin/bitcoin/pull/31437#issuecomment-2640221382
It seems that code as-is waits for wall-clock time to pass to synchronize mempools. Locally, sometimes the subtest takes a couple seconds, sometimes it takes an additional minute.
Just use mocktime?
⚠️ l0rinc opened an issue: "Networking tests fail on emulated big-endian systems"
(https://github.com/bitcoin/bitcoin/issues/31812)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
Running the test with an emulated big-endian system (via `docker run --platform linux/s390x -it ubuntu:24.10 /bin/bash`, as described in https://github.com/bitcoin/bitcoin/issues/31344#issuecomment-2493612110), I'm seeing mismatched byte orders in IP address representations.
I've noticed this in https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2614010124, but it reproduces on mas
...
(https://github.com/bitcoin/bitcoin/issues/31812)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
Running the test with an emulated big-endian system (via `docker run --platform linux/s390x -it ubuntu:24.10 /bin/bash`, as described in https://github.com/bitcoin/bitcoin/issues/31344#issuecomment-2493612110), I'm seeing mismatched byte orders in IP address representations.
I've noticed this in https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2614010124, but it reproduces on mas
...
🤔 murchandamus reviewed a pull request: "Wallet: don't underestimate the fees when spending a Taproot output"
(https://github.com/bitcoin/bitcoin/pull/26573#pullrequestreview-2599346386)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/26573#pullrequestreview-2599346386)
Concept ACK
💬 murchandamus commented on pull request "Wallet: don't underestimate the fees when spending a Taproot output":
(https://github.com/bitcoin/bitcoin/pull/26573#discussion_r1945108666)
Do I understand right that this test will try the whole battery of different script trees seen below and uses the maximal estimate across script trees, so if there are multiple different leaves that we can satisfy, our transaction might overpay, but no longer underpay?
(https://github.com/bitcoin/bitcoin/pull/26573#discussion_r1945108666)
Do I understand right that this test will try the whole battery of different script trees seen below and uses the maximal estimate across script trees, so if there are multiple different leaves that we can satisfy, our transaction might overpay, but no longer underpay?
💬 darosior commented on issue "nSequence is not set when spending from satisfiable descriptor with relative timelock":
(https://github.com/bitcoin/bitcoin/issues/31808#issuecomment-2640490507)
> I have searched the existing issues
Hmm https://github.com/bitcoin/bitcoin/issues/27527. :)
> Transaction building may be setting nLockTime / nSequence values which aren't actually compatible with what we can sign for.
To add to this, the coin selection algorithm right now may even be selecting coins that are incompatible with each other: https://github.com/bitcoin/bitcoin/issues/27526.
(https://github.com/bitcoin/bitcoin/issues/31808#issuecomment-2640490507)
> I have searched the existing issues
Hmm https://github.com/bitcoin/bitcoin/issues/27527. :)
> Transaction building may be setting nLockTime / nSequence values which aren't actually compatible with what we can sign for.
To add to this, the coin selection algorithm right now may even be selecting coins that are incompatible with each other: https://github.com/bitcoin/bitcoin/issues/27526.
💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945120612)
if `WaitMany()` returns false, we would have already waited the duration of `SELECT_TIMEOUT` -- do we need to `sleep_for()` again?
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945120612)
if `WaitMany()` returns false, we would have already waited the duration of `SELECT_TIMEOUT` -- do we need to `sleep_for()` again?
📝 vijayabhaskar78 opened a pull request: "Fix MSVC ccache reporting (Fixes #31771)"
(https://github.com/bitcoin/bitcoin/pull/31813)
Fixes #31771
### Summary
This PR resolves the incorrect reporting of `ccache` usage for MSVC builds by:
1. Removing forced compiler launcher configuration
2. Improving status message accuracy
3. Documenting proper MSVC+ccache workflows
### Key Changes
- **Deleted**: `cmake/ccache.cmake` (Eliminates conflicts with user/CI configurations like `sccache`)
- **Updated**: `CMakeLists.txt` status reporting
```cmake
if(CMAKE_CXX_COMPILER_LAUNCHER)
message(STATUS "Com
...
(https://github.com/bitcoin/bitcoin/pull/31813)
Fixes #31771
### Summary
This PR resolves the incorrect reporting of `ccache` usage for MSVC builds by:
1. Removing forced compiler launcher configuration
2. Improving status message accuracy
3. Documenting proper MSVC+ccache workflows
### Key Changes
- **Deleted**: `cmake/ccache.cmake` (Eliminates conflicts with user/CI configurations like `sccache`)
- **Updated**: `CMakeLists.txt` status reporting
```cmake
if(CMAKE_CXX_COMPILER_LAUNCHER)
message(STATUS "Com
...
💬 vijayabhaskar78 commented on pull request "Fix MSVC ccache reporting (Fixes #31771)":
(https://github.com/bitcoin/bitcoin/pull/31813#issuecomment-2640653595)
@hebasto Can you help me regarding this PR please
(https://github.com/bitcoin/bitcoin/pull/31813#issuecomment-2640653595)
@hebasto Can you help me regarding this PR please
💬 glozow commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1945226453)
Or have a test for the expected behavior? A comment in the options-handling code? I don't think it should be unspecified.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1945226453)
Or have a test for the expected behavior? A comment in the options-handling code? I don't think it should be unspecified.
💬 marcofleon commented on pull request "fuzz: add targets for PCP and NAT-PMP port mapping requests":
(https://github.com/bitcoin/bitcoin/pull/31676#discussion_r1945193198)
Should the PCP target include IPv6 or no? Something like `(domain == AF_INET || domain == AF_INET6)` might improve the test.
(https://github.com/bitcoin/bitcoin/pull/31676#discussion_r1945193198)
Should the PCP target include IPv6 or no? Something like `(domain == AF_INET || domain == AF_INET6)` might improve the test.
💬 marcofleon commented on pull request "fuzz: add targets for PCP and NAT-PMP port mapping requests":
(https://github.com/bitcoin/bitcoin/pull/31676#discussion_r1945195855)
I don't see this used anywhere.
(https://github.com/bitcoin/bitcoin/pull/31676#discussion_r1945195855)
I don't see this used anywhere.
💬 marcofleon commented on pull request "fuzz: add targets for PCP and NAT-PMP port mapping requests":
(https://github.com/bitcoin/bitcoin/pull/31676#discussion_r1945222662)
The `ConsumeData` here was the culprit for the bad coverage.
Basically, this function needs to set `name_len` to 16 or 28 for the fuzzer to continue past `SetSockAddr` in `PCPRequestPortMap`. In the PCP case, `name_len` started here as 128 and then `ConsumeData` would look to return the min of 128 or the remaining bytes of the input. It would consume the rest of the input every time because the fuzzer was optimizing for the `SetSockAddr` check. So, there would always be nothing left in `fuzz
...
(https://github.com/bitcoin/bitcoin/pull/31676#discussion_r1945222662)
The `ConsumeData` here was the culprit for the bad coverage.
Basically, this function needs to set `name_len` to 16 or 28 for the fuzzer to continue past `SetSockAddr` in `PCPRequestPortMap`. In the PCP case, `name_len` started here as 128 and then `ConsumeData` would look to return the min of 128 or the remaining bytes of the input. It would consume the rest of the input every time because the fuzzer was optimizing for the `SetSockAddr` check. So, there would always be nothing left in `fuzz
...