👍 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.
💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389240397)
nit in c14e589817035e76e2707fbcd5cb99689d42de77: If you call `Assert` before an assignment, it could make sense to turn it into a reference by calling `*Assert()`. This makes later code smaller and easier to read.
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389240397)
nit in c14e589817035e76e2707fbcd5cb99689d42de77: If you call `Assert` before an assignment, it could make sense to turn it into a reference by calling `*Assert()`. This makes later code smaller and easier to read.
💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389248596)
5f72a579de74a6d911ca8529fe8754995025d61a: Same?
Also, could squash into the previous `GetEntry` commit?
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389248596)
5f72a579de74a6d911ca8529fe8754995025d61a: Same?
Also, could squash into the previous `GetEntry` commit?
💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389292338)
91df77fde5adc7e2e785aa0d60b82a8515c9b538: Is the lock needed?
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389292338)
91df77fde5adc7e2e785aa0d60b82a8515c9b538: Is the lock needed?
💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389241558)
same
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389241558)
same
💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389293490)
nit in the first commit:
```
clang-tidy-17 -p=/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/txmempool.cpp
txmempool.cpp:848:13: error: use emplace_back instead of push_back [modernize-use-emplace,-warnings-as-errors]
848 | ret.push_back(*it);
| ^~~~~~~~~~
| emplace_back(
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389293490)
nit in the first commit:
```
clang-tidy-17 -p=/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/txmempool.cpp
txmempool.cpp:848:13: error: use emplace_back instead of push_back [modernize-use-emplace,-warnings-as-errors]
848 | ret.push_back(*it);
| ^~~~~~~~~~
| emplace_back(
💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389287348)
6635cffb1e17ad5fdfdabe68cbdcaf1c68d8070d:
Not sure about turning (theoretical) UB into an exception throw. Might be better to turn it into an assert fail by using `Assert(GetEntry())->`?
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389287348)
6635cffb1e17ad5fdfdabe68cbdcaf1c68d8070d:
Not sure about turning (theoretical) UB into an exception throw. Might be better to turn it into an assert fail by using `Assert(GetEntry())->`?
💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389274834)
nit in 1598b906a57bcd5fc50c1945d8420f8ace1d9c34: Could use `get` or `GetEntry`, when only access to the tx pointer is needed, and not full access to the internal iterator?
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389274834)
nit in 1598b906a57bcd5fc50c1945d8420f8ace1d9c34: Could use `get` or `GetEntry`, when only access to the tx pointer is needed, and not full access to the internal iterator?
💬 maflcko commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805595495)
> 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.
Why? Did you see my previous comment?
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805595495)
> 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.
Why? Did you see my previous comment?
💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1805596963)
ACK 91df77fde5adc7e2e785aa0d60b82a8515c9b538 🕐
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 91df77fde5adc7e2e785aa0d
...
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1805596963)
ACK 91df77fde5adc7e2e785aa0d60b82a8515c9b538 🕐
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 91df77fde5adc7e2e785aa0d
...
💬 dergoegge commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1805604074)
Coverage after 1000 CPU hours of fuzzing: https://dergoegge.github.io/bitcoin-coverage/pr26812/fuzz.coverage/index.html
Doesn't reach much in net_processing, so it's probably not doing much in terms of e2e testing.
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1805604074)
Coverage after 1000 CPU hours of fuzzing: https://dergoegge.github.io/bitcoin-coverage/pr26812/fuzz.coverage/index.html
Doesn't reach much in net_processing, so it's probably not doing much in terms of e2e testing.
💬 dergoegge commented on pull request "fuzz: add target for `DescriptorScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/28578#discussion_r1389308579)
We probably want to do what marco did in #28815 here, i.e. break out of the loop on "bad" data
(https://github.com/bitcoin/bitcoin/pull/28578#discussion_r1389308579)
We probably want to do what marco did in #28815 here, i.e. break out of the loop on "bad" data
💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389337349)
No, but does the other one in the `while` loop make sense?
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389337349)
No, but does the other one in the `while` loop make sense?
💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1805671306)
Will address the outstanding comments shortly.
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1805671306)
Will address the outstanding comments shortly.
💬 brunoerg commented on pull request "fuzz: add target for `DescriptorScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/28578#discussion_r1389352537)
Yes, I'm addressing it.
(https://github.com/bitcoin/bitcoin/pull/28578#discussion_r1389352537)
Yes, I'm addressing it.
💬 fjahr commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805683323)
> Why? Did you see my previous comment?
It's fine if you want to remove the randomization as well as you say in the other comment but that's not what I meant when I said "Why not reduce the values instead?". You replied to that "Yes, this patch is reducing the value.". But that is not the only thing that it does and that's why it's not what I suggested. What I suggested is reducing the max value in `seek`.
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805683323)
> Why? Did you see my previous comment?
It's fine if you want to remove the randomization as well as you say in the other comment but that's not what I meant when I said "Why not reduce the values instead?". You replied to that "Yes, this patch is reducing the value.". But that is not the only thing that it does and that's why it's not what I suggested. What I suggested is reducing the max value in `seek`.
💬 pablomartin4btc commented on pull request "script: utxo_snapshot.sh error handling":
(https://github.com/bitcoin/bitcoin/pull/27845#issuecomment-1805703495)
@hazeycode, are you still interested in continuing to work on this PR? Please let me know, otherwise I'll pick it up.
Btw, there's also another [suggestion](https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-1804941396) from @Sjors to improve the script with another validation that might be needed.
(https://github.com/bitcoin/bitcoin/pull/27845#issuecomment-1805703495)
@hazeycode, are you still interested in continuing to work on this PR? Please let me know, otherwise I'll pick it up.
Btw, there's also another [suggestion](https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-1804941396) from @Sjors to improve the script with another validation that might be needed.