💬 TheCharlatan commented on issue "Bitcoin Kernel Library Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/27587#issuecomment-3065032270)
I re-wrote the tracking issue. It now better reflects all the different areas being worked on and also surfaces a lot of the work I have not opened a pull request for.
(https://github.com/bitcoin/bitcoin/issues/27587#issuecomment-3065032270)
I re-wrote the tracking issue. It now better reflects all the different areas being worked on and also surfaces a lot of the work I have not opened a pull request for.
📝 hebasto converted_to_draft a pull request: "build: Explicitly set Qt's `AUTO{MOC,RCC,UIC}` property per target"
(https://github.com/bitcoin/bitcoin/pull/32951)
The codebase, which is compiled, consists of both files in the source tree and additional files generated during the build process. These generated files include headers such as `bitcoin-build-config.h`, headers generated for tests and benchmarks from data files, and files produced by Qt's tools, such as `moc`, `rcc` and `uic`.
When using Makefile or Ninja generators, CMake 3.31 and later provides a [convenient](https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-2991861497) builtin b
...
(https://github.com/bitcoin/bitcoin/pull/32951)
The codebase, which is compiled, consists of both files in the source tree and additional files generated during the build process. These generated files include headers such as `bitcoin-build-config.h`, headers generated for tests and benchmarks from data files, and files produced by Qt's tools, such as `moc`, `rcc` and `uic`.
When using Makefile or Ninja generators, CMake 3.31 and later provides a [convenient](https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-2991861497) builtin b
...
👋 hebasto's pull request is ready for review: "build: Explicitly set Qt's `AUTO{MOC,RCC,UIC}` property per target"
(https://github.com/bitcoin/bitcoin/pull/32951)
(https://github.com/bitcoin/bitcoin/pull/32951)
📝 hebasto converted_to_draft a pull request: "build: Explicitly set Qt's `AUTO{MOC,RCC,UIC}` property per target"
(https://github.com/bitcoin/bitcoin/pull/32951)
The codebase, which is compiled, consists of both files in the source tree and additional files generated during the build process. These generated files include headers such as `bitcoin-build-config.h`, headers generated for tests and benchmarks from data files, and files produced by Qt's tools, such as `moc`, `rcc` and `uic`.
When using Makefile or Ninja generators, CMake 3.31 and later provides a [convenient](https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-2991861497) builtin b
...
(https://github.com/bitcoin/bitcoin/pull/32951)
The codebase, which is compiled, consists of both files in the source tree and additional files generated during the build process. These generated files include headers such as `bitcoin-build-config.h`, headers generated for tests and benchmarks from data files, and files produced by Qt's tools, such as `moc`, `rcc` and `uic`.
When using Makefile or Ninja generators, CMake 3.31 and later provides a [convenient](https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-2991861497) builtin b
...
📝 hebasto opened a pull request: "[POC] ci: Skip compilation when running static code analysis"
(https://github.com/bitcoin/bitcoin/pull/32953)
This PR is a proof of concept for using a compilation database with static analysis tools such as clang-tidy or IWYU. The idea was suggested in https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-2935293250:
> Shouldn't this be excluded in the tidy CI task as well?
Additionally, this PR makes use of the `codegen` target, following the suggestion in https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-2991861497:
> It does seem nice to be able to run clang-tidy on generated file
...
(https://github.com/bitcoin/bitcoin/pull/32953)
This PR is a proof of concept for using a compilation database with static analysis tools such as clang-tidy or IWYU. The idea was suggested in https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-2935293250:
> Shouldn't this be excluded in the tidy CI task as well?
Additionally, this PR makes use of the `codegen` target, following the suggestion in https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-2991861497:
> It does seem nice to be able to run clang-tidy on generated file
...
💬 hebasto commented on pull request "doc: Remove build instruction for running `clang-tidy`":
(https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-3065305742)
> @hebasto Are you going to follow up here?
There are a few thought is https://github.com/bitcoin/bitcoin/pull/32953.
(https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-3065305742)
> @hebasto Are you going to follow up here?
There are a few thought is https://github.com/bitcoin/bitcoin/pull/32953.
💬 l0rinc commented on pull request "refactor: cleanup index logging":
(https://github.com/bitcoin/bitcoin/pull/32948#issuecomment-3065338523)
Lightweight code review ACK c18bf0bd9be632e54591493a5309e9ed4020f2e5
(https://github.com/bitcoin/bitcoin/pull/32948#issuecomment-3065338523)
Lightweight code review ACK c18bf0bd9be632e54591493a5309e9ed4020f2e5
📝 hebasto opened a pull request: "cmake: Drop no longer necessary "cmakeMinimumRequired" object"
(https://github.com/bitcoin/bitcoin/pull/32954)
The minimum required CMake version is 3.22:https://github.com/bitcoin/bitcoin/blob/6a13a6106e3c1ebe95ba6430184d6260a7b942bd/CMakeLists.txt#L10
(https://github.com/bitcoin/bitcoin/pull/32954)
The minimum required CMake version is 3.22:https://github.com/bitcoin/bitcoin/blob/6a13a6106e3c1ebe95ba6430184d6260a7b942bd/CMakeLists.txt#L10
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202647828)
For reference, this is the rust test I recently wrote: https://github.com/p2pderivatives/rust-bitcoin-coin-selection/blob/3ae36852f4263270e794c444e9fc09172997fc67/src/branch_and_bound.rs#L866
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202647828)
For reference, this is the rust test I recently wrote: https://github.com/p2pderivatives/rust-bitcoin-coin-selection/blob/3ae36852f4263270e794c444e9fc09172997fc67/src/branch_and_bound.rs#L866
💬 l0rinc commented on pull request "tests: speed up coins_tests by parallelizing":
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3065413387)
Given that the `and`s and `also`s in the description, I personally would find it useful to split the PR into smaller commits accordingly.
I'd ACK the `coins_tests.cpp` changes (had problems because of that simulation test being part of the suite numerous times), no opinion about the rest.
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3065413387)
Given that the `and`s and `also`s in the description, I personally would find it useful to split the PR into smaller commits accordingly.
I'd ACK the `coins_tests.cpp` changes (had problems because of that simulation test being part of the suite numerous times), no opinion about the rest.
💬 l0rinc commented on pull request "refactor: Avoid copies by using const references or by move-construction":
(https://github.com/bitcoin/bitcoin/pull/31650#issuecomment-3065475639)
Redid the rebase, the conflicts seem to have been solved correctly.
Changes since last ACK:
waitNext, GetMinimumTime,DeriveTarget,blockToJSON,blockheaderToJSON,GetTarget,CreateDescriptor were also less-wrong-const-ed™.
<details>
<summary>Details</summary>
```patch
diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h
index 150295e5b7..f8ae9a261f 100644
--- a/src/interfaces/mining.h
+++ b/src/interfaces/mining.h
@@ -71,7 +71,7 @@ public:
* On testnet this will addi
...
(https://github.com/bitcoin/bitcoin/pull/31650#issuecomment-3065475639)
Redid the rebase, the conflicts seem to have been solved correctly.
Changes since last ACK:
waitNext, GetMinimumTime,DeriveTarget,blockToJSON,blockheaderToJSON,GetTarget,CreateDescriptor were also less-wrong-const-ed™.
<details>
<summary>Details</summary>
```patch
diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h
index 150295e5b7..f8ae9a261f 100644
--- a/src/interfaces/mining.h
+++ b/src/interfaces/mining.h
@@ -71,7 +71,7 @@ public:
* On testnet this will addi
...
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202682814)
I just noticed a subtle bug in the test that I fixed. Let's see if anyone else notices :nerd_face:
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202682814)
I just noticed a subtle bug in the test that I fixed. Let's see if anyone else notices :nerd_face:
⚠️ anhilde opened an issue: "Bitcoin Core v29.0 incorrectly enters IBD mode when only ~600 blocks behind, preventing normal sync"
(https://github.com/bitcoin/bitcoin/issues/32955)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
Hi, I have a full node on a development machine that is not always run. It runs a self compiled none modified v29.0 node as systemd service on an Ubuntu 24.04 system. I had it twice now that the node, after not using it for a few days would go into IBD mode. This time I tried rebuilding the chain state but it would only go as far as the same block it was stuck on before. By running `bitcoi
...
(https://github.com/bitcoin/bitcoin/issues/32955)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
Hi, I have a full node on a development machine that is not always run. It runs a self compiled none modified v29.0 node as systemd service on an Ubuntu 24.04 system. I had it twice now that the node, after not using it for a few days would go into IBD mode. This time I tried rebuilding the chain state but it would only go as far as the same block it was stuck on before. By running `bitcoi
...
💬 yuvicc commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#issuecomment-3065605849)
Concept ACK
> The external consensus library was also removed while waiting for #29015. I wonder if we should remove the internal consensus library as well and drop its contents into the new internal kernel library
I agree with you, was having a similar thought on the internal library `libbitcoin_consensus` despite removal of external `libbitcoinconsensus` library. If we go this way then we can remove the `libbitcoin_consensus` from the `libraries.md` as well.
(https://github.com/bitcoin/bitcoin/pull/28690#issuecomment-3065605849)
Concept ACK
> The external consensus library was also removed while waiting for #29015. I wonder if we should remove the internal consensus library as well and drop its contents into the new internal kernel library
I agree with you, was having a similar thought on the internal library `libbitcoin_consensus` despite removal of external `libbitcoinconsensus` library. If we go this way then we can remove the `libbitcoin_consensus` from the `libraries.md` as well.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3065659959)
> as now one of your prospective counterparty in the flow can maliciously contribute an input hitting this policy limit.
This is already possible today. Besides i don't see how this is an issue: an untrusted counterparty can always make your funding protocol fail by providing invalid or non-standard inputs, or even just stopping to respond for that matter.
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3065659959)
> as now one of your prospective counterparty in the flow can maliciously contribute an input hitting this policy limit.
This is already possible today. Besides i don't see how this is an issue: an untrusted counterparty can always make your funding protocol fail by providing invalid or non-standard inputs, or even just stopping to respond for that matter.
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202762467)
I see. So `cost_of_change` is variable based on the change output size which is range bound `(0, MAX_SCRIPT_SIZE)` since the cost of creating the change output depends on the size of the change output created. I didn't know there was a max script size for an output!?
I think this type of thing might be worth adding in more detail to the commit message to help reviewers.
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202762467)
I see. So `cost_of_change` is variable based on the change output size which is range bound `(0, MAX_SCRIPT_SIZE)` since the cost of creating the change output depends on the size of the change output created. I didn't know there was a max script size for an output!?
I think this type of thing might be worth adding in more detail to the commit message to help reviewers.
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202763864)
Good call, I just noticed that as well.
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202763864)
Good call, I just noticed that as well.
🤔 Prabhat1308 reviewed a pull request: "validation: remove BLOCK_FAILED_CHILD"
(https://github.com/bitcoin/bitcoin/pull/32950#pullrequestreview-3013295386)
Concept ACK
I think removing is a better option. The only **weak** argument that we could have for not removing is that `BLOCK_FAILED_VALID` right now marks the start of the invalid block chain and is better for debugging manually. However , the burden of maintaining this distinction seems too high for debugging purposes with no actual benefit functionally.
(https://github.com/bitcoin/bitcoin/pull/32950#pullrequestreview-3013295386)
Concept ACK
I think removing is a better option. The only **weak** argument that we could have for not removing is that `BLOCK_FAILED_VALID` right now marks the start of the invalid block chain and is better for debugging manually. However , the burden of maintaining this distinction seems too high for debugging purposes with no actual benefit functionally.
💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3065733833)
Thank you for the detailed review!
I think you're right, my current approach regarding named parameter handling is too verbose/complex, and I looked at your implementation, and I really liked the way you're handling named parameters in the `RPCConvertNamedValues` function. I will implement that approach in my next update commit. Besides that, I also looked at the json parsing issue you're mentioning, and it's easily reproducible. I experimented with both the approaches, the one you provided
...
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3065733833)
Thank you for the detailed review!
I think you're right, my current approach regarding named parameter handling is too verbose/complex, and I looked at your implementation, and I really liked the way you're handling named parameters in the `RPCConvertNamedValues` function. I will implement that approach in my next update commit. Besides that, I also looked at the json parsing issue you're mentioning, and it's easily reproducible. I experimented with both the approaches, the one you provided
...
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202897142)
I see. Thanks. This will be a slightly non-uniform pattern since the window is largest for the first selection `(1, MAX_MONEY - 16)` and then the next selection will have a smaller range maybe `(1, MAX_MONEY / 2)`. So the pool will tend to have the largest UTXO's first.
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202897142)
I see. Thanks. This will be a slightly non-uniform pattern since the window is largest for the first selection `(1, MAX_MONEY - 16)` and then the next selection will have a smaller range maybe `(1, MAX_MONEY / 2)`. So the pool will tend to have the largest UTXO's first.