💬 naumenkogs commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1389028671)
8c2026848da910fdebff0a9f73e29f1f6ae81e43
So forced actually means two things to you:
1. Before connecting — something that *might* evict another peer
2. After connecting — something that *has* evicted another peer
I think this is confusing and better to use different words. RPC can expose "force_evicted" flag (to cover the latter case and apply to the `Connection`), while "forceInbound" can stay in the network/potential-connection context (the former case).
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1389028671)
8c2026848da910fdebff0a9f73e29f1f6ae81e43
So forced actually means two things to you:
1. Before connecting — something that *might* evict another peer
2. After connecting — something that *has* evicted another peer
I think this is confusing and better to use different words. RPC can expose "force_evicted" flag (to cover the latter case and apply to the `Connection`), while "forceInbound" can stay in the network/potential-connection context (the former case).
💬 maflcko commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1389093305)
Seems odd to add a TODO to add a test based on a feature that doesn't even exist.
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1389093305)
Seems odd to add a TODO to add a test based on a feature that doesn't even exist.
💬 maflcko commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1389091265)
My diff was a hack, obviously. I don't think it makes sense to escape the datadir "sandbox" and potentially overwrite user data when running a test.
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1389091265)
My diff was a hack, obviously. I don't think it makes sense to escape the datadir "sandbox" and potentially overwrite user data when running a test.
💬 maflcko commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1389094288)
nit: `blank` may not be needed? Also, could run a python formatter on this file to fix the whitespace?
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1389094288)
nit: `blank` may not be needed? Also, could run a python formatter on this file to fix the whitespace?
💬 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.