💬 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.
💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1805706401)
Thank you for the review @maflcko,
Updated 91df77fde5adc7e2e785aa0d60b82a8515c9b538 -> 3b73acb3ec678f5099aa0d77178a0b0d50670c2b ([simplifyMemPoolInteractions_14](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_14) -> [simplifyMemPoolInteractions_15](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_15), [compare](https://github.com/TheCharlatan/bitcoin/compare/simplifyMemPoolInteractions_14..simplifyMemPoolInteractions_15))
* Addressed @mafl
...
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1805706401)
Thank you for the review @maflcko,
Updated 91df77fde5adc7e2e785aa0d60b82a8515c9b538 -> 3b73acb3ec678f5099aa0d77178a0b0d50670c2b ([simplifyMemPoolInteractions_14](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_14) -> [simplifyMemPoolInteractions_15](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_15), [compare](https://github.com/TheCharlatan/bitcoin/compare/simplifyMemPoolInteractions_14..simplifyMemPoolInteractions_15))
* Addressed @mafl
...
💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389377987)
Addressed this, but did not squash, since the change is a bit different to the others.
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389377987)
Addressed this, but did not squash, since the change is a bit different to the others.
💬 L0laL33tz commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805709673)
If the test fails because the leveldb is smaller than the block files, I would propose keeping the randomization for the block files and adding a second deterministic test for leveldb?
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805709673)
If the test fails because the leveldb is smaller than the block files, I would propose keeping the randomization for the block files and adding a second deterministic test for leveldb?
💬 fjahr commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805711872)
> ./test/functional/feature_init.py --timeout-factor=.1 --randomseed=4861605774237699286
<details>
<summary>Hm, this doesn't fail for me on master.</summary>
```
$ ./test/functional/feature_init.py --timeout-factor=.1 --randomseed=4861605774237699286
2023-11-10T13:14:53.906000Z TestFramework (INFO): User supplied random seed 4861605774237699286
2023-11-10T13:14:53.907000Z TestFramework (INFO): PRNG seed is: 4861605774237699286
2023-11-10T13:14:53.907000Z TestFramework (INFO): Initiali
...
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805711872)
> ./test/functional/feature_init.py --timeout-factor=.1 --randomseed=4861605774237699286
<details>
<summary>Hm, this doesn't fail for me on master.</summary>
```
$ ./test/functional/feature_init.py --timeout-factor=.1 --randomseed=4861605774237699286
2023-11-10T13:14:53.906000Z TestFramework (INFO): User supplied random seed 4861605774237699286
2023-11-10T13:14:53.907000Z TestFramework (INFO): PRNG seed is: 4861605774237699286
2023-11-10T13:14:53.907000Z TestFramework (INFO): Initiali
...