👍 stickies-v approved a pull request: "Remove mempoolfullrbf"
(https://github.com/bitcoin/bitcoin/pull/30592#pullrequestreview-2401632616)
re-ACK 806ecd6958e4c0fe2a51bee04e20e0dca515bd40
Documentation changes to address review comments, and further functional test cleanups.
(https://github.com/bitcoin/bitcoin/pull/30592#pullrequestreview-2401632616)
re-ACK 806ecd6958e4c0fe2a51bee04e20e0dca515bd40
Documentation changes to address review comments, and further functional test cleanups.
💬 stickies-v commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1820661404)
Ah, I see. We don't mention BIP125 until further down the document, so I don't think (most) readers are expecting that symmetry here, so I still think it'd least awkward to just remove the item entirely (and renumbering the following items).
We're in bikeshed territory, so I'll stop commenting on it further.
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1820661404)
Ah, I see. We don't mention BIP125 until further down the document, so I don't think (most) readers are expecting that symmetry here, so I still think it'd least awkward to just remove the item entirely (and renumbering the following items).
We're in bikeshed territory, so I'll stop commenting on it further.
💬 stickies-v commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1820658233)
This list enumerates "BIPs that are implemented by Bitcoin Core". Adding one that isn't implemented seems confusing. I'd just remove the line entirely, we already reference BIP125 in https://github.com/bitcoin/bitcoin/blob/da10e0bab4a3e98868dd663af02c43b1dc8b7f4a/doc/policy/mempool-replacements.md?plain=1#L62
If you're worried about discoverability, adding the hyperlink there could be an alternative:
<details>
<summary>git diff on 806ecd6958</summary>
```diff
diff --git a/doc/policy/m
...
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1820658233)
This list enumerates "BIPs that are implemented by Bitcoin Core". Adding one that isn't implemented seems confusing. I'd just remove the line entirely, we already reference BIP125 in https://github.com/bitcoin/bitcoin/blob/da10e0bab4a3e98868dd663af02c43b1dc8b7f4a/doc/policy/mempool-replacements.md?plain=1#L62
If you're worried about discoverability, adding the hyperlink there could be an alternative:
<details>
<summary>git diff on 806ecd6958</summary>
```diff
diff --git a/doc/policy/m
...
💬 marcofleon commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2444038494)
Fuzzed the two targets for about 250 cpu hours each. Coverage looks good.
[txdownloadman](https://marcofleon.github.io/coverage/txdownloadman/)
[txdownloadman_impl](https://marcofleon.github.io/coverage/txdownloadmanimpl/)
Finally, the children hanging out in the recent reject filter are quiet 😌
I'll look to run the one honest peer test as well once it's ready.
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2444038494)
Fuzzed the two targets for about 250 cpu hours each. Coverage looks good.
[txdownloadman](https://marcofleon.github.io/coverage/txdownloadman/)
[txdownloadman_impl](https://marcofleon.github.io/coverage/txdownloadmanimpl/)
Finally, the children hanging out in the recent reject filter are quiet 😌
I'll look to run the one honest peer test as well once it's ready.
📝 hebasto opened a pull request: "[POC] ci: Test cross-built Windows executables on Windows natively"
(https://github.com/bitcoin/bitcoin/pull/31176)
Resolves https://github.com/bitcoin/bitcoin/issues/31071.
Things left to do:
- introduce caching
- build and test GUI
- run functional tests
(https://github.com/bitcoin/bitcoin/pull/31176)
Resolves https://github.com/bitcoin/bitcoin/issues/31071.
Things left to do:
- introduce caching
- build and test GUI
- run functional tests
💬 1440000bytes commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2444057243)
Concept ACK
https://gs.statcounter.com/os-version-market-share/windows/desktop/worldwide
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2444057243)
Concept ACK
https://gs.statcounter.com/os-version-market-share/windows/desktop/worldwide
💬 hebasto commented on pull request "[POC] ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2444060142)
@achow101 @fanquake
This PR needs the `actions/download-artifact@*` and `actions/upload-artifact@*` actions to be allowed in the repository Settings - General - Actions - General.
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2444060142)
@achow101 @fanquake
This PR needs the `actions/download-artifact@*` and `actions/upload-artifact@*` actions to be allowed in the repository Settings - General - Actions - General.
📝 polespinasa opened a pull request: "rpc, cli: return "verificationprogress" of 1 when up to date"
(https://github.com/bitcoin/bitcoin/pull/31177)
```getblockchaininfo``` never reaches 1.0 as reported in issue https://github.com/bitcoin/bitcoin/issues/31127.
This PR is based on the reviews given on https://github.com/bitcoin/bitcoin/pull/31135.
Some calls to the function ```GuessVerificationProgress``` remain unchanged pending comments on this proposal.
(https://github.com/bitcoin/bitcoin/pull/31177)
```getblockchaininfo``` never reaches 1.0 as reported in issue https://github.com/bitcoin/bitcoin/issues/31127.
This PR is based on the reviews given on https://github.com/bitcoin/bitcoin/pull/31135.
Some calls to the function ```GuessVerificationProgress``` remain unchanged pending comments on this proposal.
💬 polespinasa commented on pull request "rpc, cli: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31135#issuecomment-2444063183)
Hi, I tried to go with a proposal on this based on the comments given here https://github.com/bitcoin/bitcoin/pull/31177
(https://github.com/bitcoin/bitcoin/pull/31135#issuecomment-2444063183)
Hi, I tried to go with a proposal on this based on the comments given here https://github.com/bitcoin/bitcoin/pull/31177
📝 polespinasa converted_to_draft a pull request: "rpc, cli: return "verificationprogress" of 1 when up to date"
(https://github.com/bitcoin/bitcoin/pull/31177)
```getblockchaininfo``` never reaches 1.0 as reported in issue https://github.com/bitcoin/bitcoin/issues/31127.
This PR is based on the reviews given on https://github.com/bitcoin/bitcoin/pull/31135.
Some calls to the function ```GuessVerificationProgress``` remain unchanged pending comments on this proposal.
(https://github.com/bitcoin/bitcoin/pull/31177)
```getblockchaininfo``` never reaches 1.0 as reported in issue https://github.com/bitcoin/bitcoin/issues/31127.
This PR is based on the reviews given on https://github.com/bitcoin/bitcoin/pull/31135.
Some calls to the function ```GuessVerificationProgress``` remain unchanged pending comments on this proposal.
👍 tdb3 approved a pull request: "bench: add support for custom data directory"
(https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2401711134)
ACK ebdace5aaf74626da83eeee1f966acaad3ed0c35
Light code review and tested.
```
$ build/src/bench/bench_bitcoin -testdatadir=/mnt/31000/
...
$ ls /mnt/31000/test_common\ bitcoin/
AssembleBlock LoadExternalBlockFile ReadBlockFromDiskTest WalletCreateEncrypted
BlockAssemblerAddPackageTxns LogWithDebug ReadRawBlockFromDiskTest WalletCreatePlain
BlockFilterIndexSync LogWithThreadNames RpcMempool WalletCreateTxUseOnlyPresetInputs
...
(https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2401711134)
ACK ebdace5aaf74626da83eeee1f966acaad3ed0c35
Light code review and tested.
```
$ build/src/bench/bench_bitcoin -testdatadir=/mnt/31000/
...
$ ls /mnt/31000/test_common\ bitcoin/
AssembleBlock LoadExternalBlockFile ReadBlockFromDiskTest WalletCreateEncrypted
BlockAssemblerAddPackageTxns LogWithDebug ReadRawBlockFromDiskTest WalletCreatePlain
BlockFilterIndexSync LogWithThreadNames RpcMempool WalletCreateTxUseOnlyPresetInputs
...
💬 tdb3 commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1820702143)
nit: for awareness, may want to mention in OP that this changes directory names from 256-bit randoms to 32-bit randoms. I'm not seeing an issue with it (virtually non-existent likelihood of collision).
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1820702143)
nit: for awareness, may want to mention in OP that this changes directory names from 256-bit randoms to 32-bit randoms. I'm not seeing an issue with it (virtually non-existent likelihood of collision).
💬 hebasto commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2444103245)
> > > The cross-compiled on Ubuntu 24.10 bitcoind.exe fails to run on Windows 11 Pro 23H2.
> >
> >
> > Can you provide some actionable information? The CI and cross-compiled unit tests have run & passed.
>
> The only difference from CI is that I run `bitcoind.exe` on Windows, not under Wine.
> This seems like even more reason to do #31071.
This branch @ 7dd0ee89a092c6ec4e305fbdb0cf3afa9e41cab6 rebased on top of the https://github.com/bitcoin/bitcoin/pull/31176 fails to run `bitcoin
...
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2444103245)
> > > The cross-compiled on Ubuntu 24.10 bitcoind.exe fails to run on Windows 11 Pro 23H2.
> >
> >
> > Can you provide some actionable information? The CI and cross-compiled unit tests have run & passed.
>
> The only difference from CI is that I run `bitcoind.exe` on Windows, not under Wine.
> This seems like even more reason to do #31071.
This branch @ 7dd0ee89a092c6ec4e305fbdb0cf3afa9e41cab6 rebased on top of the https://github.com/bitcoin/bitcoin/pull/31176 fails to run `bitcoin
...
💬 l0rinc commented on pull request "doc: Extend developer-notes with file-name-only debugging fix":
(https://github.com/bitcoin/bitcoin/pull/30670#discussion_r1820749380)
Done, thanks
(https://github.com/bitcoin/bitcoin/pull/30670#discussion_r1820749380)
Done, thanks
💬 ryanofsky commented on issue "ci: intermittent issue in rpc_misc.py node0 stderr terminate called after throwing an instance of 'kj::ExceptionImpl' [15:12:14.943] what(): mp/proxy.cpp:242: disconnected: write(m_post_fd, &buffer, 1): Broken pipe [15:12:14.943] stack: 657ed083 657ed86e f7ac7d20 f7713156 f77a7e07 ":
(https://github.com/bitcoin/bitcoin/issues/31151#issuecomment-2444170080)
Assuming this is a new error, it is possible this is caused by #31105 which was merged a week ago and changed some parts of IPC shutdown sequence.
It seems this failure here is happening because some code is trying to use the IPC event loop after it is closed. Specifically some code is calling the `mp::EventLoop::removeClient` which fails writing to the event loop's local pipe at https://github.com/chaincodelabs/libmultiprocess/blob/abe254b9734f2e2b220d1456de195532d6e6ac1e/src/mp/proxy.cpp#L2
...
(https://github.com/bitcoin/bitcoin/issues/31151#issuecomment-2444170080)
Assuming this is a new error, it is possible this is caused by #31105 which was merged a week ago and changed some parts of IPC shutdown sequence.
It seems this failure here is happening because some code is trying to use the IPC event loop after it is closed. Specifically some code is calling the `mp::EventLoop::removeClient` which fails writing to the event loop's local pipe at https://github.com/chaincodelabs/libmultiprocess/blob/abe254b9734f2e2b220d1456de195532d6e6ac1e/src/mp/proxy.cpp#L2
...
👍 laanwj approved a pull request: "doc: Extend developer-notes with file-name-only debugging fix"
(https://github.com/bitcoin/bitcoin/pull/30670#pullrequestreview-2401819547)
ACK 1b0b9b4c7873ff0c6323de0c7f439466eed06049
(https://github.com/bitcoin/bitcoin/pull/30670#pullrequestreview-2401819547)
ACK 1b0b9b4c7873ff0c6323de0c7f439466eed06049
👍 hebasto approved a pull request: "Fix unsigned integer overflows in interpreter"
(https://github.com/bitcoin/bitcoin/pull/24214#pullrequestreview-2401888182)
ACK bbbbaa0d9ac9ae9c9b8109503aa30213eed543b9, I have reviewed the code and it looks OK.
A decent alternative to https://github.com/bitcoin/bitcoin/pull/29543.
(https://github.com/bitcoin/bitcoin/pull/24214#pullrequestreview-2401888182)
ACK bbbbaa0d9ac9ae9c9b8109503aa30213eed543b9, I have reviewed the code and it looks OK.
A decent alternative to https://github.com/bitcoin/bitcoin/pull/29543.
💬 ryanofsky commented on issue "ci: intermittent issue in rpc_misc.py node0 stderr terminate called after throwing an instance of 'kj::ExceptionImpl' [15:12:14.943] what(): mp/proxy.cpp:242: disconnected: write(m_post_fd, &buffer, 1): Broken pipe [15:12:14.943] stack: 657ed083 657ed86e f7ac7d20 f7713156 f77a7e07 ":
(https://github.com/bitcoin/bitcoin/issues/31151#issuecomment-2444255035)
Looking more closely at changes in #31105, I don't see anything that directly relates to this code, so it seems possible this is an older bug or a different bug. Looking up the "Broken pipe" error, it also seems like that can only be triggered if something is closing the `EventLoop::m_wait_fd` file descriptor before the `write(m_post_fd, ...)` call happens. But I don't see a code path where that would happen. May need to reproduce locally to debug. Would be curious to know if this happens more.
(https://github.com/bitcoin/bitcoin/issues/31151#issuecomment-2444255035)
Looking more closely at changes in #31105, I don't see anything that directly relates to this code, so it seems possible this is an older bug or a different bug. Looking up the "Broken pipe" error, it also seems like that can only be triggered if something is closing the `EventLoop::m_wait_fd` file descriptor before the `write(m_post_fd, ...)` call happens. But I don't see a code path where that would happen. May need to reproduce locally to debug. Would be curious to know if this happens more.
👍 TheCharlatan approved a pull request: "ci: Use clang-19 from apt.llvm.org"
(https://github.com/bitcoin/bitcoin/pull/30634#pullrequestreview-2401970706)
ACK fabe90c8242aa45a8b9925347ca6fc11d5852ffd
(https://github.com/bitcoin/bitcoin/pull/30634#pullrequestreview-2401970706)
ACK fabe90c8242aa45a8b9925347ca6fc11d5852ffd
💬 laanwj commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#issuecomment-2444317433)
> I implemented your suggestion by adding the --skip-missing-binaries option to the command.
Yes, thanks!
> I wonder if a better approach here would be to further integrate the manpage generation into the build system; given that the two issues of "which binaries exist" and "where do they exist" are already solved in that case. This should also remove the dep on git.
That would be better. The thing is, the build system isn't allowed to execute the binaries it generated (and even if so t
...
(https://github.com/bitcoin/bitcoin/pull/30986#issuecomment-2444317433)
> I implemented your suggestion by adding the --skip-missing-binaries option to the command.
Yes, thanks!
> I wonder if a better approach here would be to further integrate the manpage generation into the build system; given that the two issues of "which binaries exist" and "where do they exist" are already solved in that case. This should also remove the dep on git.
That would be better. The thing is, the build system isn't allowed to execute the binaries it generated (and even if so t
...