💬 fanquake commented on pull request " rpc/wallet: Add details and duplicate section for simulaterawtransaction":
(https://github.com/bitcoin/bitcoin/pull/25621#issuecomment-1434761587)
@anibilthare are you still working on this? Can you comment where you've addressed the review feedback. Note that at least one test is also failing.
(https://github.com/bitcoin/bitcoin/pull/25621#issuecomment-1434761587)
@anibilthare are you still working on this? Can you comment where you've addressed the review feedback. Note that at least one test is also failing.
💬 fanquake commented on pull request "Optimizations & simplifications following #25717":
(https://github.com/bitcoin/bitcoin/pull/25968#issuecomment-1434766905)
@sipa are you inteterested in adressing the review feedback here?
(https://github.com/bitcoin/bitcoin/pull/25968#issuecomment-1434766905)
@sipa are you inteterested in adressing the review feedback here?
💬 brunoerg commented on pull request "p2p: return `CSubNet` in `LookupSubNet`":
(https://github.com/bitcoin/bitcoin/pull/26078#discussion_r1109955578)
Yea, nice find, thanks!
(https://github.com/bitcoin/bitcoin/pull/26078#discussion_r1109955578)
Yea, nice find, thanks!
💬 theuni commented on pull request "build: use _FORTIFY_SOURCE=3":
(https://github.com/bitcoin/bitcoin/pull/27027#issuecomment-1434792804)
ACK 4faa4e37a6511c6ada303ef7929ac99c7462f083. After playing with this quite a bit I didn't observe any noticeable pitfalls.
I'd expect this to matter more in low-level code, so I'm interested to see how much #27118 affects the numbers.
(https://github.com/bitcoin/bitcoin/pull/27027#issuecomment-1434792804)
ACK 4faa4e37a6511c6ada303ef7929ac99c7462f083. After playing with this quite a bit I didn't observe any noticeable pitfalls.
I'd expect this to matter more in low-level code, so I'm interested to see how much #27118 affects the numbers.
💬 theuni commented on pull request "depends: harden libevent":
(https://github.com/bitcoin/bitcoin/pull/27118#issuecomment-1434795409)
Do you have a before/after count for hardened functions?
(https://github.com/bitcoin/bitcoin/pull/27118#issuecomment-1434795409)
Do you have a before/after count for hardened functions?
💬 fanquake commented on pull request "contrib: add ELF OS ABI check to symbol-check.py":
(https://github.com/bitcoin/bitcoin/pull/26953#issuecomment-1434803116)
Need to follow up here in regards to powerpc64 vs le. le is currently 3.10.0:
```bash
ELF 64-bit LSB pie executable, 64-bit PowerPC or cisco 7500, OpenPOWER ELF V2 ABI, version 1 (GNU/Linux), dynamically linked, interpreter /lib64/ld64.so.2, for GNU/Linux 3.10.0, with debug_info, not stripped
```
as opposed to non-le:
```bash
ELF 64-bit MSB pie executable, 64-bit PowerPC or cisco 7500, Power ELF V1 ABI, version 1 (GNU/Linux), dynamically linked, interpreter /lib64/ld64.so.1, for GNU/Linux
...
(https://github.com/bitcoin/bitcoin/pull/26953#issuecomment-1434803116)
Need to follow up here in regards to powerpc64 vs le. le is currently 3.10.0:
```bash
ELF 64-bit LSB pie executable, 64-bit PowerPC or cisco 7500, OpenPOWER ELF V2 ABI, version 1 (GNU/Linux), dynamically linked, interpreter /lib64/ld64.so.2, for GNU/Linux 3.10.0, with debug_info, not stripped
```
as opposed to non-le:
```bash
ELF 64-bit MSB pie executable, 64-bit PowerPC or cisco 7500, Power ELF V1 ABI, version 1 (GNU/Linux), dynamically linked, interpreter /lib64/ld64.so.1, for GNU/Linux
...
💬 fanquake commented on pull request "depends: harden libevent":
(https://github.com/bitcoin/bitcoin/pull/27118#issuecomment-1434820512)
> Do you have a before/after count for hardened functions?
Using GCC 13 and glibc 2.37 (with only branch):
```bash
# master
nm -C src/bitcoind | grep _chk
U __fprintf_chk@GLIBC_2.17
U __memcpy_chk@GLIBC_2.17
U __snprintf_chk@GLIBC_2.17
U __stack_chk_fail@GLIBC_2.17
U __stack_chk_guard@GLIBC_2.17
U __vsnprintf_chk@GLIBC_2.17
objdump -d src/bitcoind | grep "_chk@plt" | wc -l
32
#
...
(https://github.com/bitcoin/bitcoin/pull/27118#issuecomment-1434820512)
> Do you have a before/after count for hardened functions?
Using GCC 13 and glibc 2.37 (with only branch):
```bash
# master
nm -C src/bitcoind | grep _chk
U __fprintf_chk@GLIBC_2.17
U __memcpy_chk@GLIBC_2.17
U __snprintf_chk@GLIBC_2.17
U __stack_chk_fail@GLIBC_2.17
U __stack_chk_guard@GLIBC_2.17
U __vsnprintf_chk@GLIBC_2.17
objdump -d src/bitcoind | grep "_chk@plt" | wc -l
32
#
...
💬 MarcoFalke commented on pull request "lint: enable E722 do not use bare except":
(https://github.com/bitcoin/bitcoin/pull/25867#issuecomment-1434851767)
Yeah, I was thinking it would be easier to replace them with `except Exception`, like you did initially. This would mean the review is easy, because it can only affect `KeyError`, right? With several exceptions listed, we'd have to review the whole call stack, which is burdensome, no?
(https://github.com/bitcoin/bitcoin/pull/25867#issuecomment-1434851767)
Yeah, I was thinking it would be easier to replace them with `except Exception`, like you did initially. This would mean the review is easy, because it can only affect `KeyError`, right? With several exceptions listed, we'd have to review the whole call stack, which is burdensome, no?
💬 willcl-ark commented on issue "TransactionError::ALREADY_IN_CHAIN inconsistency ":
(https://github.com/bitcoin/bitcoin/issues/19363#issuecomment-1434852432)
Although it is inconsistent I slightly prefer the idea of returning the most accurate information available to the user.
Following this logic instead of removing this error type, we could take two new actions to try and increase the chance that the caller gets more accurate information back:
1. Inside `BroadcastTransaction()` add a call to `GetTransaction()` on the condition that the user has `txindex` enabled
2. Augment [`HandleATMPError`](https://github.com/bitcoin/bitcoin/blob/fe1b3256
...
(https://github.com/bitcoin/bitcoin/issues/19363#issuecomment-1434852432)
Although it is inconsistent I slightly prefer the idea of returning the most accurate information available to the user.
Following this logic instead of removing this error type, we could take two new actions to try and increase the chance that the caller gets more accurate information back:
1. Inside `BroadcastTransaction()` add a call to `GetTransaction()` on the condition that the user has `txindex` enabled
2. Augment [`HandleATMPError`](https://github.com/bitcoin/bitcoin/blob/fe1b3256
...
⚠️ pinheadmz opened an issue: "on OSX, bitcoind chooses different data directory than Bitcoin-Qt"
(https://github.com/bitcoin/bitcoin/issues/27119)
bitcoind uses `~/Library/Application Support/Bitcoin` as base data directory path but Bitcoin-Qt uses `~/.bitcoin`
I actually much prefer the latter since it is more UNIX-like and I assume users running the daemon from the command line probably do as well. It might make sense if this behavior was swapped and the GUI chose the user-friendly library/support path, but my preference would be using `~/.bitcoin` on OSX all the time.
The issue most likely comes down to this logic in system.cpp:
...
(https://github.com/bitcoin/bitcoin/issues/27119)
bitcoind uses `~/Library/Application Support/Bitcoin` as base data directory path but Bitcoin-Qt uses `~/.bitcoin`
I actually much prefer the latter since it is more UNIX-like and I assume users running the daemon from the command line probably do as well. It might make sense if this behavior was swapped and the GUI chose the user-friendly library/support path, but my preference would be using `~/.bitcoin` on OSX all the time.
The issue most likely comes down to this logic in system.cpp:
...
💬 sipa commented on pull request "Optimizations & simplifications following #25717":
(https://github.com/bitcoin/bitcoin/pull/25968#discussion_r1110031910)
How could it be faster? It's just a function that runs the provided lambda in a loop...?
(https://github.com/bitcoin/bitcoin/pull/25968#discussion_r1110031910)
How could it be faster? It's just a function that runs the provided lambda in a loop...?
💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#issuecomment-1434867163)
Since last comment:
- Rebased on-top of the latest version of #27021.
- Addressed that `CalculateTotalBumpFee` now returns an std::optional<CAmount> because we automatically fail calculation for (too) large clusters of unconfirmed transactions
- Waiting for #27021 before opening for review
(https://github.com/bitcoin/bitcoin/pull/26152#issuecomment-1434867163)
Since last comment:
- Rebased on-top of the latest version of #27021.
- Addressed that `CalculateTotalBumpFee` now returns an std::optional<CAmount> because we automatically fail calculation for (too) large clusters of unconfirmed transactions
- Waiting for #27021 before opening for review
💬 glozow commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1434870979)
review-beg-pinging @LarryRuane @josibake @stickies-v who have looked at `MiniMiner` previously
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1434870979)
review-beg-pinging @LarryRuane @josibake @stickies-v who have looked at `MiniMiner` previously
👍 hebasto approved a pull request: "refactor: wallet, remove global 'ArgsManager' dependency"
(https://github.com/bitcoin/bitcoin/pull/26889)
re-ACK 52f4d567d69425dfd514489079db80483024a80d
(https://github.com/bitcoin/bitcoin/pull/26889)
re-ACK 52f4d567d69425dfd514489079db80483024a80d
👍 hebasto approved a pull request: "refactor: remove windows-only compat.h usage in random"
(https://github.com/bitcoin/bitcoin/pull/26814)
re-ACK 621cfb77227b5a240d66547947f73130f0c51f44, rebased only since my [recent](https://github.com/bitcoin/bitcoin/pull/26814#pullrequestreview-1237312144) review. Verified with:
```
git range-diff master 4e7bed297a6dd1ae0da52a7eb99659849f18f259 621cfb77227b5a240d66547947f73130f0c51f44
```
(https://github.com/bitcoin/bitcoin/pull/26814)
re-ACK 621cfb77227b5a240d66547947f73130f0c51f44, rebased only since my [recent](https://github.com/bitcoin/bitcoin/pull/26814#pullrequestreview-1237312144) review. Verified with:
```
git range-diff master 4e7bed297a6dd1ae0da52a7eb99659849f18f259 621cfb77227b5a240d66547947f73130f0c51f44
```
👍 ryanofsky approved a pull request: "validation: Improve error handling when VerifyDB dosn't finish successfully"
(https://github.com/bitcoin/bitcoin/pull/25574)
Code review ACK 0af16e7134459e0820ab95d751093876c1ec4c6d. Only small suggested changes since the last review, like renaming some of the enum values. I did leave more suggestions, but they are not very important and could be followups
(https://github.com/bitcoin/bitcoin/pull/25574)
Code review ACK 0af16e7134459e0820ab95d751093876c1ec4c6d. Only small suggested changes since the last review, like renaming some of the enum values. I did leave more suggestions, but they are not very important and could be followups
💬 ryanofsky commented on pull request "validation: Improve error handling when VerifyDB dosn't finish successfully":
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1110031541)
In commit "init, validation: Improve handling if VerifyDB() fails due to insufficient dbcache" (0c7785bb2540b69564104767d38342704230cbc2)
Might be worth adding a comment to explain why you would set the option. Maybe:
```c++
//! Setting require_full_verification to true will require all checks at
//! check_level (below) to succeed for loading to succeed. Setting it to
//! false will skip checks if cache is not big enough to run them, so may be
//! helpful for running with a small cache
...
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1110031541)
In commit "init, validation: Improve handling if VerifyDB() fails due to insufficient dbcache" (0c7785bb2540b69564104767d38342704230cbc2)
Might be worth adding a comment to explain why you would set the option. Maybe:
```c++
//! Setting require_full_verification to true will require all checks at
//! check_level (below) to succeed for loading to succeed. Setting it to
//! false will skip checks if cache is not big enough to run them, so may be
//! helpful for running with a small cache
...
💬 ryanofsky commented on pull request "validation: Improve error handling when VerifyDB dosn't finish successfully":
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1110050930)
In commit "validation: return VerifyDBResult::INTERRUPTED if verification was interrupted" (d6f781f1cfcbc2c2ad5ee289a0642ed00386d013)
It would be less surprising to have `VerifyDBResult::INTERRUPTED` return `ChainstateLoadStatus::INTERRUPTED` than `ChainstateLoadStatus::SUCCESS`. Another potentially good side effect would be that it would avoid the caller printing a misleading load time if the load didn't really succeed:
https://github.com/bitcoin/bitcoin/blob/fe1b3256888bd0e70d0c9655f565e
...
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1110050930)
In commit "validation: return VerifyDBResult::INTERRUPTED if verification was interrupted" (d6f781f1cfcbc2c2ad5ee289a0642ed00386d013)
It would be less surprising to have `VerifyDBResult::INTERRUPTED` return `ChainstateLoadStatus::INTERRUPTED` than `ChainstateLoadStatus::SUCCESS`. Another potentially good side effect would be that it would avoid the caller printing a misleading load time if the load didn't really succeed:
https://github.com/bitcoin/bitcoin/blob/fe1b3256888bd0e70d0c9655f565e
...
💬 ryanofsky commented on pull request "validation: Improve error handling when VerifyDB dosn't finish successfully":
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1110037026)
In commit "validation: Change return value of VerifyDB to enum type" (6360b5302d2675788de5c4a28ea77d823f6d809e)
Commit message says this does not change behavior, but this is changing log prints slightly replacing `VerifyDB():` prefix with `Verification error:`. This is probably a good change, but could be clarified in the commit message.
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1110037026)
In commit "validation: Change return value of VerifyDB to enum type" (6360b5302d2675788de5c4a28ea77d823f6d809e)
Commit message says this does not change behavior, but this is changing log prints slightly replacing `VerifyDB():` prefix with `Verification error:`. This is probably a good change, but could be clarified in the commit message.
💬 ryanofsky commented on pull request "validation: Improve error handling when VerifyDB dosn't finish successfully":
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1109999734)
re: https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1107700526
Nice, it's good that returning in one place makes it easier to see what the precedence is, or change what's returned in the future.
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1109999734)
re: https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1107700526
Nice, it's good that returning in one place makes it easier to see what the precedence is, or change what's returned in the future.
💬 ryanofsky commented on pull request "validation: Improve error handling when VerifyDB dosn't finish successfully":
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1110019015)
re: https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1109110569
Thanks for implementing the suggestion. This comment was not about `bitcoind`, but about code that uses libbitcoinkernel such as:
https://github.com/bitcoin/bitcoin/blob/fe1b3256888bd0e70d0c9655f565e139ec87b606/src/bitcoin-chainstate.cpp#L93-L96
Code like this has it's own options and defaults, and is not aware of `checkblocks` or `checklevel`. It is just calling and using the `LoadChainstate` function directly. Fr
...
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1110019015)
re: https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1109110569
Thanks for implementing the suggestion. This comment was not about `bitcoind`, but about code that uses libbitcoinkernel such as:
https://github.com/bitcoin/bitcoin/blob/fe1b3256888bd0e70d0c9655f565e139ec87b606/src/bitcoin-chainstate.cpp#L93-L96
Code like this has it's own options and defaults, and is not aware of `checkblocks` or `checklevel`. It is just calling and using the `LoadChainstate` function directly. Fr
...