💬 maflcko commented on pull request "test: Check error details with assert_debug_log on the assumeutxo invalid hash dump - follow-up #28698":
(https://github.com/bitcoin/bitcoin/pull/28835#issuecomment-1805334263)
lgtm ACK 7de76853728b423339d17f39224cf20305da1832
(https://github.com/bitcoin/bitcoin/pull/28835#issuecomment-1805334263)
lgtm ACK 7de76853728b423339d17f39224cf20305da1832
💬 naiyoma commented on pull request "doc: Add example of mixing private and public keys in descriptors":
(https://github.com/bitcoin/bitcoin/pull/28373#discussion_r1389124162)
I did make the changes suggested above
(https://github.com/bitcoin/bitcoin/pull/28373#discussion_r1389124162)
I did make the changes suggested above
🚀 fanquake merged a pull request: "test: Check error details with assert_debug_log on the assumeutxo invalid hash dump - follow-up #28698"
(https://github.com/bitcoin/bitcoin/pull/28835)
(https://github.com/bitcoin/bitcoin/pull/28835)
💬 fanquake commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805441141)
cc @L0laL33tz @theStack
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805441141)
cc @L0laL33tz @theStack
💬 fjahr commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805454319)
Can you link to the intermittent failures? Why not reduce the values instead?
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805454319)
Can you link to the intermittent failures? Why not reduce the values instead?
💬 maflcko commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805471333)
> Can you link to the intermittent failures? Why not reduce the values instead?
You can simply run the test in a loop to see it fail. For example:
```
./test/functional/feature_init.py --timeout-factor=.1 --randomseed=4861605774237699286
...
AssertionError: [node 0] bitcoind should have exited within 6s with expected error Error opening block database.
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805471333)
> Can you link to the intermittent failures? Why not reduce the values instead?
You can simply run the test in a loop to see it fail. For example:
```
./test/functional/feature_init.py --timeout-factor=.1 --randomseed=4861605774237699286
...
AssertionError: [node 0] bitcoind should have exited within 6s with expected error Error opening block database.
💬 maflcko commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805472756)
> Why not reduce the values instead?
Yes, this patch is reducing the value.
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805472756)
> Why not reduce the values instead?
Yes, this patch is reducing the value.
💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389223982)
first commit: I think here the result can not be nullptr, so a reference makes more sense, no?
diff:
```diff
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index e019ab9595..3930280797 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -806,8 +806,8 @@ public:
{
if (!m_node.mempool) return;
LOCK2(::cs_main, m_node.mempool->cs);
- for (const auto& entry : m_node.mempool->entryAll()) {
- notifications.trans
...
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389223982)
first commit: I think here the result can not be nullptr, so a reference makes more sense, no?
diff:
```diff
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index e019ab9595..3930280797 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -806,8 +806,8 @@ public:
{
if (!m_node.mempool) return;
LOCK2(::cs_main, m_node.mempool->cs);
- for (const auto& entry : m_node.mempool->entryAll()) {
- notifications.trans
...
💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389227863)
Thanks, will apply.
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389227863)
Thanks, will apply.
💬 maflcko commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805491397)
Also, on a second thought, I don't really understand the goal of the randomized byte corruptions. Are they supposed to increase the test coverage for different code paths? If yes, I think it makes sense to list those code paths. Otherwise, if only a single error-code-path is covered, I think a deterministic test is better.
I understand that the good first issue mentioned that advanced perturbations should be tested as well, but may require pulling in a leveldb parser. I don't think it makes s
...
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805491397)
Also, on a second thought, I don't really understand the goal of the randomized byte corruptions. Are they supposed to increase the test coverage for different code paths? If yes, I think it makes sense to list those code paths. Otherwise, if only a single error-code-path is covered, I think a deterministic test is better.
I understand that the good first issue mentioned that advanced perturbations should be tested as well, but may require pulling in a leveldb parser. I don't think it makes s
...
💬 glozow commented on pull request "MiniMiner changes for package linearization":
(https://github.com/bitcoin/bitcoin/pull/28762#discussion_r1389238484)
There are 2 constructors, one requires mempool (and its lock) and the other one doesn't.
(https://github.com/bitcoin/bitcoin/pull/28762#discussion_r1389238484)
There are 2 constructors, one requires mempool (and its lock) and the other one doesn't.
💬 glozow commented on pull request "MiniMiner changes for package linearization":
(https://github.com/bitcoin/bitcoin/pull/28762#discussion_r1389239646)
Oh I see this is not in the best place - perhaps this should be in the docstring for the with-mempool ctor.
(https://github.com/bitcoin/bitcoin/pull/28762#discussion_r1389239646)
Oh I see this is not in the best place - perhaps this should be in the docstring for the with-mempool ctor.
💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1805501889)
Thank you for the patch @maflcko,
Updated f2d8e17851da215d0b85ffad9d80e50eb7b2b363 -> 91df77fde5adc7e2e785aa0d60b82a8515c9b538 ([simplifyMemPoolInteractions_13](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_13) -> [simplifyMemPoolInteractions_14](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_14), [compare](https://github.com/TheCharlatan/bitcoin/compare/simplifyMemPoolInteractions_13..simplifyMemPoolInteractions_14))
* Applied @maflcko
...
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1805501889)
Thank you for the patch @maflcko,
Updated f2d8e17851da215d0b85ffad9d80e50eb7b2b363 -> 91df77fde5adc7e2e785aa0d60b82a8515c9b538 ([simplifyMemPoolInteractions_13](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_13) -> [simplifyMemPoolInteractions_14](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_14), [compare](https://github.com/TheCharlatan/bitcoin/compare/simplifyMemPoolInteractions_13..simplifyMemPoolInteractions_14))
* Applied @maflcko
...
💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389245944)
This seems a bit out of scope of this PR, but I think you are right, I don't see the need for `::cs_main` here either. Maybe open a separate PR for this?
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389245944)
This seems a bit out of scope of this PR, but I think you are right, I don't see the need for `::cs_main` here either. Maybe open a separate PR for this?
👍 hebasto approved a pull request: "Bugfix: configure: Correct check for fuzz binary needing a main function"
(https://github.com/bitcoin/bitcoin/pull/28564#pullrequestreview-1724552391)
ACK 8f6ca584cd5c0b4882ce6f3c158b6f23aed8f6dc.
The `./configure` script logs are as follow:
- in the master branch:
```
checking whether main function is needed for fuzz binary... checking whether the linker accepts ... no
yes
```
or
```
checking whether main function is needed for fuzz binary... checking whether the linker accepts ... yes
no
```
- in this PR branch:
```
checking whether main function is needed for fuzz binary... yes
```
or
```
checking whether main function i
...
(https://github.com/bitcoin/bitcoin/pull/28564#pullrequestreview-1724552391)
ACK 8f6ca584cd5c0b4882ce6f3c158b6f23aed8f6dc.
The `./configure` script logs are as follow:
- in the master branch:
```
checking whether main function is needed for fuzz binary... checking whether the linker accepts ... no
yes
```
or
```
checking whether main function is needed for fuzz binary... checking whether the linker accepts ... yes
no
```
- in this PR branch:
```
checking whether main function is needed for fuzz binary... yes
```
or
```
checking whether main function i
...
📝 TheCharlatan opened a pull request: "[refactor] Remove BlockAssembler m_mempool member"
(https://github.com/bitcoin/bitcoin/pull/28843)
Instead, pass the mempool pointer as an argument to CreateNewBlock and pass that on to addPackageTxs. Before, addPackageTxs had access to both the m_mempool member and the passed in mempool, which could be confusing. This was noticed in this PR review: https://github.com/bitcoin/bitcoin/pull/25223#discussion_r898164322.
(https://github.com/bitcoin/bitcoin/pull/28843)
Instead, pass the mempool pointer as an argument to CreateNewBlock and pass that on to addPackageTxs. Before, addPackageTxs had access to both the m_mempool member and the passed in mempool, which could be confusing. This was noticed in this PR review: https://github.com/bitcoin/bitcoin/pull/25223#discussion_r898164322.
👍 maflcko approved a pull request: "[refactor] Remove BlockAssembler m_mempool member"
(https://github.com/bitcoin/bitcoin/pull/28843#pullrequestreview-1724561168)
lgtm
(https://github.com/bitcoin/bitcoin/pull/28843#pullrequestreview-1724561168)
lgtm
💬 maflcko commented on pull request "Bugfix: configure: Correct check for fuzz binary needing a main function":
(https://github.com/bitcoin/bitcoin/pull/28564#issuecomment-1805531566)
So this is only to fix a test-only debug-only output? Not sure. I guess it can be merged, if cmake does not make it into 27.x. Otherwise it seems odd to change this, when the code will be removed either way before any user will even run or see it.
(https://github.com/bitcoin/bitcoin/pull/28564#issuecomment-1805531566)
So this is only to fix a test-only debug-only output? Not sure. I guess it can be merged, if cmake does not make it into 27.x. Otherwise it seems odd to change this, when the code will be removed either way before any user will even run or see it.
👋 TheCharlatan's pull request is ready for review: "[refactor] Remove BlockAssembler m_mempool member"
(https://github.com/bitcoin/bitcoin/pull/28843)
(https://github.com/bitcoin/bitcoin/pull/28843)
💬 fjahr commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805593065)
> Yes, this patch is reducing the value.
Maybe as a side-effect but it's rolling back the randomization completely. What I mean is reducing the max value of `seek`, i.e. 15000 to something lower that is safe.
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805593065)
> Yes, this patch is reducing the value.
Maybe as a side-effect but it's rolling back the randomization completely. What I mean is reducing the max value of `seek`, i.e. 15000 to something lower that is safe.