👋 hodlinator's pull request is ready for review: "util: Improve documentation and negation of args"
(https://github.com/bitcoin/bitcoin/pull/31212)
(https://github.com/bitcoin/bitcoin/pull/31212)
💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2485490965)
> I'd like to review this, but first, I'd like to understand why it is still in draft form.
Had a linefeed issue on Windows in the previous push (and was in a WIP state over the weekend, before that).. but pretty sure it's ready to go.
(https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2485490965)
> I'd like to review this, but first, I'd like to understand why it is still in draft form.
Had a linefeed issue on Windows in the previous push (and was in a WIP state over the weekend, before that).. but pretty sure it's ready to go.
💬 Sjors commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2485492404)
I wrote:
> I set export `BITCOIND=bitcoin-node` just for the ARM job for now.
I think it should be enough to do this for one depends job and one non-depends job. I'll adjust that later.
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2485492404)
I wrote:
> I set export `BITCOIND=bitcoin-node` just for the ARM job for now.
I think it should be enough to do this for one depends job and one non-depends job. I'll adjust that later.
👍 TheCharlatan approved a pull request: "multiprocess: Add capnp wrapper for Chain interface"
(https://github.com/bitcoin/bitcoin/pull/29409#pullrequestreview-2445205614)
lgtm ACK 34d3e2a6eaaab9de5328c3e64739f1392696c7db
(https://github.com/bitcoin/bitcoin/pull/29409#pullrequestreview-2445205614)
lgtm ACK 34d3e2a6eaaab9de5328c3e64739f1392696c7db
💬 fanquake commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2485516817)
Apparently code coverage on oss-fuzz has been broken since CMake. See https://issues.oss-fuzz.com/issues/379122777. Full logs (https://oss-fuzz-build-logs.storage.googleapis.com/log-6b12c5ff-d6c4-441c-a10f-2dffff96e82c.txt):
```bash
Step #7: [0m [0;31merror: /workspace/out/libfuzzer-coverage-x86_64/src/bitcoin-core/build_fuzz/src/wallet/wallet/walletdb.h: No such file or directory
Step #7: [0m [0;31mwarning: The file '/src/bitcoin-core/build_fuzz/src/wallet/wallet/walletdb.h' isn't covered.
...
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2485516817)
Apparently code coverage on oss-fuzz has been broken since CMake. See https://issues.oss-fuzz.com/issues/379122777. Full logs (https://oss-fuzz-build-logs.storage.googleapis.com/log-6b12c5ff-d6c4-441c-a10f-2dffff96e82c.txt):
```bash
Step #7: [0m [0;31merror: /workspace/out/libfuzzer-coverage-x86_64/src/bitcoin-core/build_fuzz/src/wallet/wallet/walletdb.h: No such file or directory
Step #7: [0m [0;31mwarning: The file '/src/bitcoin-core/build_fuzz/src/wallet/wallet/walletdb.h' isn't covered.
...
💬 fanquake commented on issue "bitcoin-qt failed assertion on startup":
(https://github.com/bitcoin/bitcoin/issues/31289#issuecomment-2485535623)
I guess move to the GUI repo? Unclear if this will be fixed or not. @hebasto.
(https://github.com/bitcoin/bitcoin/issues/31289#issuecomment-2485535623)
I guess move to the GUI repo? Unclear if this will be fixed or not. @hebasto.
💬 brunoerg commented on pull request "wallet: remove BDB dependency from wallet migration benchmark":
(https://github.com/bitcoin/bitcoin/pull/31241#discussion_r1848241863)
In f7dbb300d7a4d18c9d85f80e5fdd7e5bcd21c6f0: Do we really need `TestLoadWallet` here? Couldn't we simply create the wallet directly (e.g. `std::unique_ptr<CWallet> wallet_ptr{std::make_unique<CWallet>(test_setup->m_node.chain.get()), "", CreateMockableWalletDatabase())};`)?
(https://github.com/bitcoin/bitcoin/pull/31241#discussion_r1848241863)
In f7dbb300d7a4d18c9d85f80e5fdd7e5bcd21c6f0: Do we really need `TestLoadWallet` here? Couldn't we simply create the wallet directly (e.g. `std::unique_ptr<CWallet> wallet_ptr{std::make_unique<CWallet>(test_setup->m_node.chain.get()), "", CreateMockableWalletDatabase())};`)?
👍 BrandonOdiwuor approved a pull request: "cmake: Improve robustness and usability"
(https://github.com/bitcoin/bitcoin/pull/31233#pullrequestreview-2445321566)
tACK 4b6a842c28010a00e121fd36cc0b4e1fa658d249 on Ubuntu 24.04
Was able to successfully test that `util_test_runner` and `util_rpcauth_test` are correctly disabled when Python3 interpreter is not found (screenshots below)
Overally this is an improvement, using the `Python3::Interpreter` target defined directly by find_package(...) is cleaner than relying on the `${PYTHON_COMMAND}` variable we define
without Python3
tACK 4b6a842c28010a00e121fd36cc0b4e1fa658d249 on Ubuntu 24.04
Was able to successfully test that `util_test_runner` and `util_rpcauth_test` are correctly disabled when Python3 interpreter is not found (screenshots below)
Overally this is an improvement, using the `Python3::Interpreter` target defined directly by find_package(...) is cleaner than relying on the `${PYTHON_COMMAND}` variable we define
without Python3
![Screenshot from 2024-11-19 15-24-13](https://github.com/user-attachmen
...
💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2485623740)
(The current Windows CI failure ([internal compiler error](https://github.com/bitcoin/bitcoin/actions/runs/11910989737/job/33191550567?pr=31212#step:10:1278)) is not specific to this PR, see #31303).
(https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2485623740)
(The current Windows CI failure ([internal compiler error](https://github.com/bitcoin/bitcoin/actions/runs/11910989737/job/33191550567?pr=31212#step:10:1278)) is not specific to this PR, see #31303).
📝 defitricks opened a pull request: "Typo Update bitcoin-conf.md"
(https://github.com/bitcoin/bitcoin/pull/31322)
This pull request corrects a minor but important typographical error in the documentation. In the section discussing the placement of comments in the `bitcoin.conf` configuration file, the word **"preferable"** was used incorrectly. The correct word in this context is **"preferred"**.
- **Before:** "Comments may appear in two ways: on their own on an otherwise empty line (_preferable_);"
- **After:** "Comments may appear in two ways: on their own on an otherwise empty line (_preferred_);"
...
(https://github.com/bitcoin/bitcoin/pull/31322)
This pull request corrects a minor but important typographical error in the documentation. In the section discussing the placement of comments in the `bitcoin.conf` configuration file, the word **"preferable"** was used incorrectly. The correct word in this context is **"preferred"**.
- **Before:** "Comments may appear in two ways: on their own on an otherwise empty line (_preferable_);"
- **After:** "Comments may appear in two ways: on their own on an otherwise empty line (_preferred_);"
...
💬 TheCharlatan commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#issuecomment-2485653947)
I'm not sure why this is really an improvement. Can you elaborate on why no external user would want to pass in a script pubkey directly, instead of manually manipulating the coinbase in the block template?
(https://github.com/bitcoin/bitcoin/pull/31318#issuecomment-2485653947)
I'm not sure why this is really an improvement. Can you elaborate on why no external user would want to pass in a script pubkey directly, instead of manually manipulating the coinbase in the block template?
💬 Sjors commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2485712161)
I added a commit to make `m_tip_block` and `std::optional`.
Also, looking at `m_tip_block` again, I'm fairly sure it's set to the tip during node initialization. Otherwise we'd never get something wrong:
https://github.com/bitcoin/bitcoin/blob/746f93b4f0f47c67642057944fb79dddf17369f9/src/init.cpp#L1794-L1806
So I was probably doing something wrong earlier.
I dropped the workaround. An early return only happens if:
1. `m_tip_block` is set; AND
2. `m_tip_block.value()` is different f
...
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2485712161)
I added a commit to make `m_tip_block` and `std::optional`.
Also, looking at `m_tip_block` again, I'm fairly sure it's set to the tip during node initialization. Otherwise we'd never get something wrong:
https://github.com/bitcoin/bitcoin/blob/746f93b4f0f47c67642057944fb79dddf17369f9/src/init.cpp#L1794-L1806
So I was probably doing something wrong earlier.
I dropped the workaround. An early return only happens if:
1. `m_tip_block` is set; AND
2. `m_tip_block.value()` is different f
...
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2485715952)
`c4f51c7f61...2b0103705f`: rebase and make the virtual methods of `SockMan` private since they are only used by `SockMan`.
About storing the `CNode` in `SockMan` (which would be templated like `SockMan<CNode>`) and making the communication between `SockMan` and the higher class (e.g. `CConnman`) based on `CNode` instead of node id: this is an excellent idea that will make the code more straight-forward and the higher classes simpler. However it would make this PR larger. I will leave it off f
...
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2485715952)
`c4f51c7f61...2b0103705f`: rebase and make the virtual methods of `SockMan` private since they are only used by `SockMan`.
About storing the `CNode` in `SockMan` (which would be templated like `SockMan<CNode>`) and making the communication between `SockMan` and the higher class (e.g. `CConnman`) based on `CNode` instead of node id: this is an excellent idea that will make the code more straight-forward and the higher classes simpler. However it would make this PR larger. I will leave it off f
...
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2485727139)
Marking this draft, let's review the early return and m_tip_block block changes in #31297 first.
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2485727139)
Marking this draft, let's review the early return and m_tip_block block changes in #31297 first.
📝 Sjors converted_to_draft a pull request: "Add waitNext() to BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/31283)
Alternative approach to #31003, suggested in https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2451942362.
This PR introduces `waitNext()`. It waits for either the tip to update or for fees at the top of the mempool to rise sufficiently. It then returns a new template, with which the caller can rinse and repeat.
(https://github.com/bitcoin/bitcoin/pull/31283)
Alternative approach to #31003, suggested in https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2451942362.
This PR introduces `waitNext()`. It waits for either the tip to update or for fees at the top of the mempool to rise sufficiently. It then returns a new template, with which the caller can rinse and repeat.
👍 TheCharlatan approved a pull request: "Prune mining interface"
(https://github.com/bitcoin/bitcoin/pull/31196#pullrequestreview-2445511412)
ACK a67ba64f8f63a0f9f2c86d72cf7240bee7c5388c
(https://github.com/bitcoin/bitcoin/pull/31196#pullrequestreview-2445511412)
ACK a67ba64f8f63a0f9f2c86d72cf7240bee7c5388c
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1848395907)
I also have a goal of eliminating the need to invoke `CMPA` on a changeset anyway. In #28676, I believe the only place that `CMPA` is used in a changeset is for determining whether an RBF might be replacing something it depends on. I think it should be straightforward to have the changeset logic directly test for that condition and return failure if a dependency is being added to something that is slated for removal, which would then allow us to remove the method altogether.
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1848395907)
I also have a goal of eliminating the need to invoke `CMPA` on a changeset anyway. In #28676, I believe the only place that `CMPA` is used in a changeset is for determining whether an RBF might be replacing something it depends on. I think it should be straightforward to have the changeset logic directly test for that condition and return failure if a dependency is being added to something that is slated for removal, which would then allow us to remove the method altogether.
🤔 hodlinator requested changes to a pull request: "test: Revert to random path element"
(https://github.com/bitcoin/bitcoin/pull/31317#pullrequestreview-2445541192)
Reviewed fa80b08fef0eaa600339caa678fdf80a8aec3ce3
(https://github.com/bitcoin/bitcoin/pull/31317#pullrequestreview-2445541192)
Reviewed fa80b08fef0eaa600339caa678fdf80a8aec3ce3
💬 hodlinator commented on pull request "test: Revert to random path element":
(https://github.com/bitcoin/bitcoin/pull/31317#discussion_r1848404959)
Tested cherrypicking 8e5f8a4f775b93d30b86d28405886eea232cf875 from #30737 on top of this PR to forward `RANDOM_CTX_SEED` on inside *test/fuzz/test_runner.py*.
Ran this ~10 times:
```
RANDOM_CTX_SEED=21 build_fuzz/test/fuzz/test_runner.py ../qa-assets/
```
Many of the failures come down to:
```
Done 2 runs in 0 second(s)
terminate called after throwing an instance of 'std::filesystem::__cxx11::filesystem_error'
what(): filesystem error: cannot remove all: No such file or directory [
...
(https://github.com/bitcoin/bitcoin/pull/31317#discussion_r1848404959)
Tested cherrypicking 8e5f8a4f775b93d30b86d28405886eea232cf875 from #30737 on top of this PR to forward `RANDOM_CTX_SEED` on inside *test/fuzz/test_runner.py*.
Ran this ~10 times:
```
RANDOM_CTX_SEED=21 build_fuzz/test/fuzz/test_runner.py ../qa-assets/
```
Many of the failures come down to:
```
Done 2 runs in 0 second(s)
terminate called after throwing an instance of 'std::filesystem::__cxx11::filesystem_error'
what(): filesystem error: cannot remove all: No such file or directory [
...
💬 ryanofsky commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#discussion_r1848409060)
In commit "mining: add early return to waitTipChanged()" (71a539c5c3ae165be71a5c8d7f5b1860f8261bd1)
Does this change have any effect? It looks like this is basically changing `cv.wait_for(condition)` to `if (!condition) cv.wait_for(condition)` which I would expect to be equivalent.
(https://github.com/bitcoin/bitcoin/pull/31297#discussion_r1848409060)
In commit "mining: add early return to waitTipChanged()" (71a539c5c3ae165be71a5c8d7f5b1860f8261bd1)
Does this change have any effect? It looks like this is basically changing `cv.wait_for(condition)` to `if (!condition) cv.wait_for(condition)` which I would expect to be equivalent.