💬 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
...
💬 pablomartin4btc commented on pull request "[do not merge] validation: assumeutxo params mainnet":
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-1805715439)
> As discussed offline, the `utxo_snapshot.sh` script should have a sanity check to make the node isn't pruned below the snapshot height. On a pruned node you can only make a snapshot for a block that hasn't been pruned yet.
I agree with it, mentioned on a #27845 that's touching the script ATM.
In the testing I performed and mentioned above the pruned node was above the snapshot height (`800'000`):
```
"pruned": true,
"pruneheight": 600510,
"automatic_pruning": true,
"prune_
...
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-1805715439)
> As discussed offline, the `utxo_snapshot.sh` script should have a sanity check to make the node isn't pruned below the snapshot height. On a pruned node you can only make a snapshot for a block that hasn't been pruned yet.
I agree with it, mentioned on a #27845 that's touching the script ATM.
In the testing I performed and mentioned above the pruned node was above the snapshot height (`800'000`):
```
"pruned": true,
"pruneheight": 600510,
"automatic_pruning": true,
"prune_
...
👍 dergoegge approved a pull request: "[refactor] Remove BlockAssembler m_mempool member"
(https://github.com/bitcoin/bitcoin/pull/28843#pullrequestreview-1724777099)
Code review ACK ed71a7b9e1e67d6155fe1def5acf51beb12c9baf
(https://github.com/bitcoin/bitcoin/pull/28843#pullrequestreview-1724777099)
Code review ACK ed71a7b9e1e67d6155fe1def5acf51beb12c9baf
💬 brunoerg commented on pull request "fuzz: add target for `DescriptorScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/28578#discussion_r1389412045)
done
(https://github.com/bitcoin/bitcoin/pull/28578#discussion_r1389412045)
done
💬 brunoerg commented on pull request "fuzz: add target for `DescriptorScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/28578#issuecomment-1805755558)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/28578#discussion_r1389308579
(https://github.com/bitcoin/bitcoin/pull/28578#issuecomment-1805755558)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/28578#discussion_r1389308579
💬 brunoerg commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1389415296)
In 0b8143396183d23e215fc90a2d24813ad1b7ad23: nit
```suggestion
for _ in range(100):
```
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1389415296)
In 0b8143396183d23e215fc90a2d24813ad1b7ad23: nit
```suggestion
for _ in range(100):
```
💬 maflcko commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805763774)
> Hm, this doesn't fail for me on master.
Jup, that is what I mean when I say we don't want to pull in a leveldb dependency into this test, unless there is a reason. leveldb has different code paths depending on the operating system. You are using macOS and I was using Linux. I presume you'll be able to reproduce if you also use Linux. Alternatively, you can try to run the test in a loop on macOS, but I can't do that, because I don't have macOS.
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805763774)
> Hm, this doesn't fail for me on master.
Jup, that is what I mean when I say we don't want to pull in a leveldb dependency into this test, unless there is a reason. leveldb has different code paths depending on the operating system. You are using macOS and I was using Linux. I presume you'll be able to reproduce if you also use Linux. Alternatively, you can try to run the test in a loop on macOS, but I can't do that, because I don't have macOS.
📝 fanquake opened a pull request: "contrib: drop GCC MAX_VERSION to 4.3.0 in symbol-check"
(https://github.com/bitcoin/bitcoin/pull/28844)
Reflect the actual symbols used, i.e:
```bash
bitcoind: symbol __bswapsi2 from unsupported version GCC_4.3.0(7)
```
Guix build (aarch64):
```bash
c279cbbd49ce93756706bb32c20555b257da8aca3c8e4615de9d9e51f13430ea guix-build-5a6be635dfaf/output/aarch64-linux-gnu/SHA256SUMS.part
5f8020098a63de0fe60e4bcb8eb55c975cd5c7522c9c3813ffa6acb616ff4500 guix-build-5a6be635dfaf/output/aarch64-linux-gnu/bitcoin-5a6be635dfaf-aarch64-linux-gnu-debug.tar.gz
c20907aa6cf92d8bda541a43583b3a61bc18fbcd2efc
...
(https://github.com/bitcoin/bitcoin/pull/28844)
Reflect the actual symbols used, i.e:
```bash
bitcoind: symbol __bswapsi2 from unsupported version GCC_4.3.0(7)
```
Guix build (aarch64):
```bash
c279cbbd49ce93756706bb32c20555b257da8aca3c8e4615de9d9e51f13430ea guix-build-5a6be635dfaf/output/aarch64-linux-gnu/SHA256SUMS.part
5f8020098a63de0fe60e4bcb8eb55c975cd5c7522c9c3813ffa6acb616ff4500 guix-build-5a6be635dfaf/output/aarch64-linux-gnu/bitcoin-5a6be635dfaf-aarch64-linux-gnu-debug.tar.gz
c20907aa6cf92d8bda541a43583b3a61bc18fbcd2efc
...
💬 maflcko commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805788724)
> 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?
Yes, I considered this and I think it could make sense. However, it may require reading the file size and using that as the maximum, or other changes. So for now, I made it trivial to review by just reverting, so that the CI and tests are no longer failing intermittently.
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805788724)
> 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?
Yes, I considered this and I think it could make sense. However, it may require reading the file size and using that as the maximum, or other changes. So for now, I made it trivial to review by just reverting, so that the CI and tests are no longer failing intermittently.
💬 hazeycode commented on pull request "script: utxo_snapshot.sh error handling":
(https://github.com/bitcoin/bitcoin/pull/27845#issuecomment-1805800276)
Sorry for the delayed response @pablomartin4btc. Thanks for looking at this. Unfortunately, I don't have a time to work on this further at the moment, so feel free to take baton.
(https://github.com/bitcoin/bitcoin/pull/27845#issuecomment-1805800276)
Sorry for the delayed response @pablomartin4btc. Thanks for looking at this. Unfortunately, I don't have a time to work on this further at the moment, so feel free to take baton.
💬 glozow commented on pull request "fuzz: Minor improvements to tx_package_eval target":
(https://github.com/bitcoin/bitcoin/pull/28825#discussion_r1389450123)
is this supposed to be empty?
(https://github.com/bitcoin/bitcoin/pull/28825#discussion_r1389450123)
is this supposed to be empty?
👍 theStack approved a pull request: "test: Avoid intermittent failures in feature_init"
(https://github.com/bitcoin/bitcoin/pull/28831#pullrequestreview-1724874769)
> 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?
That was also my initial thought when reading this PR's description. In contrast to the chainstate leveldb files, the block files are fully in our control and possible faulty behaviour caused by perturbation should (hopefully) be identical an all operating systems, as there is no external library involved.
...
(https://github.com/bitcoin/bitcoin/pull/28831#pullrequestreview-1724874769)
> 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?
That was also my initial thought when reading this PR's description. In contrast to the chainstate leveldb files, the block files are fully in our control and possible faulty behaviour caused by perturbation should (hopefully) be identical an all operating systems, as there is no external library involved.
...
✅ glozow closed a pull request: "rpc: permit any ancestor package through submitpackage"
(https://github.com/bitcoin/bitcoin/pull/28813)
(https://github.com/bitcoin/bitcoin/pull/28813)
💬 Ayush170-Future commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1389464021)
This is out of the blue, but the reason I struggle to compile after each change is that I'm currently using a Windows 10 OS. And I can't run Wallet Fuzz tests on my machine for some unknown reason. I've attempted Fuzz tests outside of Wallet code, and they run fine. I've also experimented with other Windows machines, and this issue seems to persists across all of them.
So, I've been using an Ubuntu VM for code testing. I've gone through [Makefile.test.include](https://github.com/bitcoin/bitco
...
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1389464021)
This is out of the blue, but the reason I struggle to compile after each change is that I'm currently using a Windows 10 OS. And I can't run Wallet Fuzz tests on my machine for some unknown reason. I've attempted Fuzz tests outside of Wallet code, and they run fine. I've also experimented with other Windows machines, and this issue seems to persists across all of them.
So, I've been using an Ubuntu VM for code testing. I've gone through [Makefile.test.include](https://github.com/bitcoin/bitco
...
💬 pablomartin4btc commented on pull request "script: utxo_snapshot.sh error handling":
(https://github.com/bitcoin/bitcoin/pull/27845#issuecomment-1805827248)
> Sorry for the delayed response @pablomartin4btc. Thanks for looking at this. Unfortunately, I don't have a time to work on this further at the moment, so feel free to take baton.
Thanks @hazeycode, no worries, please close this PR, I'll create a new one and will add you as a co-author in the commit. Cheers!
(https://github.com/bitcoin/bitcoin/pull/27845#issuecomment-1805827248)
> Sorry for the delayed response @pablomartin4btc. Thanks for looking at this. Unfortunately, I don't have a time to work on this further at the moment, so feel free to take baton.
Thanks @hazeycode, no worries, please close this PR, I'll create a new one and will add you as a co-author in the commit. Cheers!
✅ glozow closed a pull request: "validate package transactions with their in-package ancestor sets"
(https://github.com/bitcoin/bitcoin/pull/26711)
(https://github.com/bitcoin/bitcoin/pull/26711)
✅ fanquake closed a pull request: "script: utxo_snapshot.sh error handling"
(https://github.com/bitcoin/bitcoin/pull/27845)
(https://github.com/bitcoin/bitcoin/pull/27845)