💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425866501)
`best_peers.emplace(hash_key, indexed_state.first);` (avoids a temporary `std::pair` object in the caller).
(it'd become `best_peers.emplace_back(hash_key indexed_state.first);` if `best_peers` is converted to a vector as suggested above.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425866501)
`best_peers.emplace(hash_key, indexed_state.first);` (avoids a temporary `std::pair` object in the caller).
(it'd become `best_peers.emplace_back(hash_key indexed_state.first);` if `best_peers` is converted to a vector as suggested above.
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425865073)
An alternative which may be slightly faster (unsure, depends on how fast `std::modf` is):
`const size_t targets_size = ((deterministic_randomizer_with_wtxid.Finalize() & 0xFFFFFFFF) + uint64_t(limit * 0x100000000)) >> 32;`
It has lower precision, but I suspect 32 bits is more than sufficient here.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425865073)
An alternative which may be slightly faster (unsure, depends on how fast `std::modf` is):
`const size_t targets_size = ((deterministic_randomizer_with_wtxid.Finalize() & 0xFFFFFFFF) + uint64_t(limit * 0x100000000)) >> 32;`
It has lower precision, but I suspect 32 bits is more than sufficient here.
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425856345)
I believe the appropriate type in this case is `CSipHasher&& deterministic_randomizer`. This avoids a copy, allows the local variable inside `ShouldFanoutTo` to be modified, and the caller is constructing a fresh `CSipHasher` anyway.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425856345)
I believe the appropriate type in this case is `CSipHasher&& deterministic_randomizer`. This avoids a copy, allows the local variable inside `ShouldFanoutTo` to be modified, and the caller is constructing a fresh `CSipHasher` anyway.
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425870404)
Use `const auto& [cur_peer_id, cur_peer] : m_peer_map` here. You're making a copy of the `PeerRef` object for every iteration here (which isn't the end of the world, it's just a shared pointer, but copying those does involve an atomic memory operation).
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425870404)
Use `const auto& [cur_peer_id, cur_peer] : m_peer_map` here. You're making a copy of the `PeerRef` object for every iteration here (which isn't the end of the world, it's just a shared pointer, but copying those does involve an atomic memory operation).
🤔 amitiuttarwar reviewed a pull request: "p2p: attempt to fill full outbound connection slots with peers that support tx relay"
(https://github.com/bitcoin/bitcoin/pull/28538#pullrequestreview-1780458476)
ACK d636e38d79a4c3950da91090b1f787163f11e24d
(https://github.com/bitcoin/bitcoin/pull/28538#pullrequestreview-1780458476)
ACK d636e38d79a4c3950da91090b1f787163f11e24d
💬 amitiuttarwar commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1425865265)
hm, I think this comment can be a bit misleading for people who don't readily have the full context. the setup tests peers who set the version relay bool to false, indicating they don't want tx messages. if they are running bitcoind, that means they are running in blocksonly mode, but from the POV of the tests we don't actually know that. I think it could be helpful to clarify that in the comment here, and then can continue to use the var naming of "blocksonly" in the test.
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1425865265)
hm, I think this comment can be a bit misleading for people who don't readily have the full context. the setup tests peers who set the version relay bool to false, indicating they don't want tx messages. if they are running bitcoind, that means they are running in blocksonly mode, but from the POV of the tests we don't actually know that. I think it could be helpful to clarify that in the comment here, and then can continue to use the var naming of "blocksonly" in the test.
💬 amitiuttarwar commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1425878915)
> I added a refactor commit renaming ForEachNode() to ForEachFullyConnectedNode() and removing the duplicate checks / comments.
I like it, I find it clear
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1425878915)
> I added a refactor commit renaming ForEachNode() to ForEachFullyConnectedNode() and removing the duplicate checks / comments.
I like it, I find it clear
💬 aureleoules commented on pull request "bench: wallet, fix change position out of range error":
(https://github.com/bitcoin/bitcoin/pull/29065#issuecomment-1854701221)
> Maybe it is a bug in your g++?
Right maybe, I just tested with `g++ 13.2.0` and it runs fine now. No worries then.
(https://github.com/bitcoin/bitcoin/pull/29065#issuecomment-1854701221)
> Maybe it is a bug in your g++?
Right maybe, I just tested with `g++ 13.2.0` and it runs fine now. No worries then.
💬 maflcko commented on pull request "fuzz: set `m_fallback_fee` and `m_fee_mode` in `wallet_fees` target":
(https://github.com/bitcoin/bitcoin/pull/29076#issuecomment-1854708178)
Could squash this easy change into one commit?
(https://github.com/bitcoin/bitcoin/pull/29076#issuecomment-1854708178)
Could squash this easy change into one commit?
💬 brunoerg commented on pull request "fuzz: set `m_fallback_fee` and `m_fee_mode` in `wallet_fees` target":
(https://github.com/bitcoin/bitcoin/pull/29076#issuecomment-1854718328)
> Could squash this easy change into one commit?
Sure, done!
(https://github.com/bitcoin/bitcoin/pull/29076#issuecomment-1854718328)
> Could squash this easy change into one commit?
Sure, done!
💬 shovas commented on issue "Use of a wallet shouldn't be blocked in prune mode ("wallet loading failed... beyond pruned data")":
(https://github.com/bitcoin/bitcoin/issues/27188#issuecomment-1854796784)
> Would be good to know why your node is aborting on a bad block. Maybe you have a storage issue? Could you share the logs? (here or in a new issue is fine, just tag me when you do it).
So far I have been using USB storage (128GB SanDisk Extreme Pro USB and a 1TB ADATA external ssd) whereas I have now successfully downloaded the complete blockchain to internal nvme fine, something that failed many times when trying to use a usb device as storage. Two machines, both Windows 10.
I am running
...
(https://github.com/bitcoin/bitcoin/issues/27188#issuecomment-1854796784)
> Would be good to know why your node is aborting on a bad block. Maybe you have a storage issue? Could you share the logs? (here or in a new issue is fine, just tag me when you do it).
So far I have been using USB storage (128GB SanDisk Extreme Pro USB and a 1TB ADATA external ssd) whereas I have now successfully downloaded the complete blockchain to internal nvme fine, something that failed many times when trying to use a usb device as storage. Two machines, both Windows 10.
I am running
...
💬 kashifs commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1425998172)
Perhaps. That would make it consistent with const CAmount& selection_target. It's my understanding that the documentation should exactly match the function definition. Is there another way to look at this?
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1425998172)
Perhaps. That would make it consistent with const CAmount& selection_target. It's my understanding that the documentation should exactly match the function definition. Is there another way to look at this?
💬 murchandamus commented on pull request "fuzz: coinselection, improve `min_viable_change`/`change_output_size`":
(https://github.com/bitcoin/bitcoin/pull/28372#issuecomment-1855053056)
Just kicked off 10×10h of fuzzing, will take a look tomorrow
(https://github.com/bitcoin/bitcoin/pull/28372#issuecomment-1855053056)
Just kicked off 10×10h of fuzzing, will take a look tomorrow
💬 maflcko commented on pull request "[WIP] C++20 std::views::reverse":
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1855414135)
Interesting that the guix macOS build passed, given that it is on clang-15, no?
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1855414135)
Interesting that the guix macOS build passed, given that it is on clang-15, no?
💬 maflcko commented on pull request "fuzz: set `m_fallback_fee` and `m_fee_mode` in `wallet_fees` target":
(https://github.com/bitcoin/bitcoin/pull/29076#issuecomment-1855419653)
GHA fail can be ignored:
```
==> Pouring python@3.11--3.11.6_1.ventura.bottle.tar.gz
Error: The `brew link` step did not complete successfully
The formula built, but is not symlinked into /usr/local
Could not symlink bin/2to3
Target /usr/local/bin/2to3
already exists. You may want to remove it:
rm '/usr/local/bin/2to3'
To force the link and overwrite all conflicting files:
brew link --overwrite python@3.11
To list all files that would be deleted:
brew link --overwrite --d
...
(https://github.com/bitcoin/bitcoin/pull/29076#issuecomment-1855419653)
GHA fail can be ignored:
```
==> Pouring python@3.11--3.11.6_1.ventura.bottle.tar.gz
Error: The `brew link` step did not complete successfully
The formula built, but is not symlinked into /usr/local
Could not symlink bin/2to3
Target /usr/local/bin/2to3
already exists. You may want to remove it:
rm '/usr/local/bin/2to3'
To force the link and overwrite all conflicting files:
brew link --overwrite python@3.11
To list all files that would be deleted:
brew link --overwrite --d
...
💬 maflcko commented on pull request "fuzz: set `m_fallback_fee` and `m_fee_mode` in `wallet_fees` target":
(https://github.com/bitcoin/bitcoin/pull/29076#issuecomment-1855420108)
review ACK e03d6f7ed534f423f58236866f8e83beee1871e1
(https://github.com/bitcoin/bitcoin/pull/29076#issuecomment-1855420108)
review ACK e03d6f7ed534f423f58236866f8e83beee1871e1
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1426435808)
Yeah... slightly annoying that we can't check this until we're sure a trim happened.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1426435808)
Yeah... slightly annoying that we can't check this until we're sure a trim happened.
💬 vasild commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1426447476)
> confusing to have a u8string method that doesn't actually return a u8string
My concern exactly!
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1426447476)
> confusing to have a u8string method that doesn't actually return a u8string
My concern exactly!
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#issuecomment-1855476801)
Before @sipa optimization:
```
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 1,401,785.00 | 713.38 | 1.6% | 0.02 | `ShouldFanoutTo`
```
After:
```
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 369,620.50 | 2,705.48
...
(https://github.com/bitcoin/bitcoin/pull/28765#issuecomment-1855476801)
Before @sipa optimization:
```
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 1,401,785.00 | 713.38 | 1.6% | 0.02 | `ShouldFanoutTo`
```
After:
```
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 369,620.50 | 2,705.48
...
💬 vasild commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1426458856)
The warning tells us that the code is _potentially_ unsafe. Not that it has a certain bug now. It is similar to having a variable declared with `GUARDED_BY()`, then accessing it without the mutex in such a way that it is safe.
> Maybe declaring the fs::path& path const, and writing to m_cached_blocks_path directly could make the warning go away?
Would that be like tricking the compiler, going around the warning without actually removing the underlying potential problem?
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1426458856)
The warning tells us that the code is _potentially_ unsafe. Not that it has a certain bug now. It is similar to having a variable declared with `GUARDED_BY()`, then accessing it without the mutex in such a way that it is safe.
> Maybe declaring the fs::path& path const, and writing to m_cached_blocks_path directly could make the warning go away?
Would that be like tricking the compiler, going around the warning without actually removing the underlying potential problem?