💬 willcl-ark commented on pull request "[DO NOT MERGE] Schnorr batch verification for blocks":
(https://github.com/bitcoin/bitcoin/pull/29491#issuecomment-2106978586)
> batch validation is attempted and decreases performance even before taproot activated
From a quick skim of the changes ISTM that we always use the `BatchingCachingTransactionSignatureChecker`, and there was no switch to the `CachingTransactionSignatureChecker`, but this could well be because this is only in WIP state. It doesnt account for why it's currently slower in all cases though.
(https://github.com/bitcoin/bitcoin/pull/29491#issuecomment-2106978586)
> batch validation is attempted and decreases performance even before taproot activated
From a quick skim of the changes ISTM that we always use the `BatchingCachingTransactionSignatureChecker`, and there was no switch to the `CachingTransactionSignatureChecker`, but this could well be because this is only in WIP state. It doesnt account for why it's currently slower in all cases though.
💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1598090674)
I just checked two things:
1. LevelDB does not support indexes. It also does not support SQL queries.
If we were to store the amount (of the largest taproot output) in the index, any value filtering has to be done at the time we query the index. Worst case 9000 entries per block (a 1 input 1 output taproot transaction is 111 vbyte). That might be fine, you'd have to benchmark.
If it _is_ a bottleneck, then we could refactor the base index to allow sqlite3 as a backend.
2. The only wa
...
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1598090674)
I just checked two things:
1. LevelDB does not support indexes. It also does not support SQL queries.
If we were to store the amount (of the largest taproot output) in the index, any value filtering has to be done at the time we query the index. Worst case 9000 entries per block (a 1 input 1 output taproot transaction is 111 vbyte). That might be fine, you'd have to benchmark.
If it _is_ a bottleneck, then we could refactor the base index to allow sqlite3 as a backend.
2. The only wa
...
💬 josibake commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1598091393)
I don't see the value of running a test like this? The code here is not the definition of `H`, just one way of calculating the value, based on the definition in BIP341. For example, if the test were to fail, that would just mean the comment is out of date, not that our value for `H` is incorrect.
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1598091393)
I don't see the value of running a test like this? The code here is not the definition of `H`, just one way of calculating the value, based on the definition in BIP341. For example, if the test were to fail, that would just mean the comment is out of date, not that our value for `H` is incorrect.
💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1598094935)
Using `IsDust()` makes no different for the regular index: https://github.com/Sjors/bitcoin/commit/20aba5dfa34224432787ac450b8e9d3a66d8eea9
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1598094935)
Using `IsDust()` makes no different for the regular index: https://github.com/Sjors/bitcoin/commit/20aba5dfa34224432787ac450b8e9d3a66d8eea9
💬 josibake commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1598099088)
Trying to keep this a move-only refactor, so would prefer to leave it for now. I end up touching this code again in #28201; will make a note of this for that PR.
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1598099088)
Trying to keep this a move-only refactor, so would prefer to leave it for now. I end up touching this code again in #28201; will make a note of this for that PR.
💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2107001990)
Fixed the Windows build error, but drafted while based on #30078.
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2107001990)
Fixed the Windows build error, but drafted while based on #30078.
📝 fanquake converted_to_draft a pull request: "depends: build zeromq with CMake"
(https://github.com/bitcoin/bitcoin/pull/29723)
Based on #30078.
This picks up a change, which is a switch to building zeromq with CMake. It includes a patch with a couple changes I've upstreamed:
* https://github.com/zeromq/libzmq/pull/4668
* https://github.com/zeromq/libzmq/pull/4670
and another Windows-related change:
* https://github.com/zeromq/libzmq/pull/4678
I also noticed the CMake Windows version autodetction was broken with mingw-w64 (https://github.com/zeromq/libzmq/issues/4669), so we set the version explicitly.
(https://github.com/bitcoin/bitcoin/pull/29723)
Based on #30078.
This picks up a change, which is a switch to building zeromq with CMake. It includes a patch with a couple changes I've upstreamed:
* https://github.com/zeromq/libzmq/pull/4668
* https://github.com/zeromq/libzmq/pull/4670
and another Windows-related change:
* https://github.com/zeromq/libzmq/pull/4678
I also noticed the CMake Windows version autodetction was broken with mingw-w64 (https://github.com/zeromq/libzmq/issues/4669), so we set the version explicitly.
💬 josibake commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1598105196)
Would prefer to not change for now to keep this a move-only commit.
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1598105196)
Would prefer to not change for now to keep this a move-only commit.
💬 fanquake commented on pull request "[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP":
(https://github.com/bitcoin/bitcoin/pull/29790#discussion_r1598107518)
Should remove `-DHARDENING=OFF` here.
(https://github.com/bitcoin/bitcoin/pull/29790#discussion_r1598107518)
Should remove `-DHARDENING=OFF` here.
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1598114179)
I don't think it adds much here? We are using BOOST_CHECK to to ensure the function call succeeds before proceeding, so getting a line number failure for BOOST_CHECK seems sufficient for debugging. Also, if dealing with valid keys, this function should never fail.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1598114179)
I don't think it adds much here? We are using BOOST_CHECK to to ensure the function call succeeds before proceeding, so getting a line number failure for BOOST_CHECK seems sufficient for debugging. Also, if dealing with valid keys, this function should never fail.
💬 ajtowns commented on issue "Compilation failure with `-O0` + `-fsanitize=address` due to inline asm":
(https://github.com/bitcoin/bitcoin/issues/29801#issuecomment-2107018954)
How about adding something like:
```diff
--- a/configure.ac
+++ b/configure.ac
@@ -349,7 +349,14 @@ esac
if test "$enable_debug" = "yes"; then
dnl Disable all optimizations
- AX_CHECK_COMPILE_FLAG([-O0], [DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -O0"], [], [$CXXFLAG_WERROR])
+ case "$use_sanitizers" in
+ *address*)
+ AX_CHECK_COMPILE_FLAG([-Og], [DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -Og"], [], [$CXXFLAG_WERROR])
+ ;;
+ *)
+ AX_CHECK_COMPILE_FLAG([-O0], [DEBUG_CXXFL
...
(https://github.com/bitcoin/bitcoin/issues/29801#issuecomment-2107018954)
How about adding something like:
```diff
--- a/configure.ac
+++ b/configure.ac
@@ -349,7 +349,14 @@ esac
if test "$enable_debug" = "yes"; then
dnl Disable all optimizations
- AX_CHECK_COMPILE_FLAG([-O0], [DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -O0"], [], [$CXXFLAG_WERROR])
+ case "$use_sanitizers" in
+ *address*)
+ AX_CHECK_COMPILE_FLAG([-Og], [DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -Og"], [], [$CXXFLAG_WERROR])
+ ;;
+ *)
+ AX_CHECK_COMPILE_FLAG([-O0], [DEBUG_CXXFL
...
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1598119960)
I don't really see a benefit. We shouldn't be exposing this value to anything outside the class, so defining a constexpr here seems unnecessarily verbose.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1598119960)
I don't really see a benefit. We shouldn't be exposing this value to anything outside the class, so defining a constexpr here seems unnecessarily verbose.
💬 ajtowns commented on issue "contrib/signet/miner: miner won't consider --nbits parameter":
(https://github.com/bitcoin/bitcoin/issues/30091#issuecomment-2107073436)
> So the 2:30 block times are expected then, and at the first difficulty adjustment I would be expecting the difficulty to increase to such a point that it would take my CPU 600 seconds to generate a block.
I think you've got NBITS=1d00ac8d, which if my math is correct is ~1300 times more difficult than the min signet difficulty, which should take ~5 retarget periods to achieve. The 6th one would run at about 7m46s per block; for a total of ~28 days to ramp up the difficulty. (If you backdate
...
(https://github.com/bitcoin/bitcoin/issues/30091#issuecomment-2107073436)
> So the 2:30 block times are expected then, and at the first difficulty adjustment I would be expecting the difficulty to increase to such a point that it would take my CPU 600 seconds to generate a block.
I think you've got NBITS=1d00ac8d, which if my math is correct is ~1300 times more difficult than the min signet difficulty, which should take ~5 retarget periods to achieve. The 6th one would run at about 7m46s per block; for a total of ~28 days to ramp up the difficulty. (If you backdate
...
💬 maflcko commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-2107074521)
utACK d7290d662f494503f28e087dd728b492c0bb2c5f
(https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-2107074521)
utACK d7290d662f494503f28e087dd728b492c0bb2c5f
✅ remyers closed a pull request: "wallet: Target a pre-defined utxo set composition by adjusting change outputs"
(https://github.com/bitcoin/bitcoin/pull/29442)
(https://github.com/bitcoin/bitcoin/pull/29442)
💬 remyers commented on pull request "wallet: Target a pre-defined utxo set composition by adjusting change outputs":
(https://github.com/bitcoin/bitcoin/pull/29442#issuecomment-2107079658)
Replaced by simpler #30080
(https://github.com/bitcoin/bitcoin/pull/29442#issuecomment-2107079658)
Replaced by simpler #30080
👍 josibake approved a pull request: "wallet: Implement independent BDB parser"
(https://github.com/bitcoin/bitcoin/pull/26606#pullrequestreview-2052111060)
ACK https://github.com/bitcoin/bitcoin/pull/26606/commits/a0943b812ef38826a4ee2732af5f24e470281117
Spent some time comparing this to the bdb header files and LGTM. Test coverage also looks good. Left a few nits regarding comments. Overall, I do think we should prompt users to file an issue specifically for this if loading a database fails, considering it might be nothing wrong with their database and instead an edge case being hit in the parser.
(https://github.com/bitcoin/bitcoin/pull/26606#pullrequestreview-2052111060)
ACK https://github.com/bitcoin/bitcoin/pull/26606/commits/a0943b812ef38826a4ee2732af5f24e470281117
Spent some time comparing this to the bdb header files and LGTM. Test coverage also looks good. Left a few nits regarding comments. Overall, I do think we should prompt users to file an issue specifically for this if loading a database fails, considering it might be nothing wrong with their database and instead an edge case being hit in the parser.
💬 josibake commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1598149611)
in https://github.com/bitcoin/bitcoin/pull/26606/commits/e643cac230fc2ba059847bd4e196bddfd97b1e8e ("Add MakeBerkeleyRODatabase"):
Seems like we should have some sort of call to action here prompting the user to open an issue / reach out on IRC if this fails? If we fail to open a bdb database here, the database itself could be fine and it instead might be an issue with our parser.
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1598149611)
in https://github.com/bitcoin/bitcoin/pull/26606/commits/e643cac230fc2ba059847bd4e196bddfd97b1e8e ("Add MakeBerkeleyRODatabase"):
Seems like we should have some sort of call to action here prompting the user to open an issue / reach out on IRC if this fails? If we fail to open a bdb database here, the database itself could be fine and it instead might be an issue with our parser.
💬 josibake commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1598137085)
in https://github.com/bitcoin/bitcoin/pull/26606/commits/5f0cddb4f66f2a174babdfb17bc31a3011e5cc20 ("wallet: implement independent BDB deserializer in BerkeleyRODatabase"):
nit: I somewhat know whats going here on based on previous discussions about this PR, but without that prior context this being commented out is pretty confusing and I don't think the comment above provides enough explanation. Perhaps add more context in the comment or just remove this snippet entirely?
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1598137085)
in https://github.com/bitcoin/bitcoin/pull/26606/commits/5f0cddb4f66f2a174babdfb17bc31a3011e5cc20 ("wallet: implement independent BDB deserializer in BerkeleyRODatabase"):
nit: I somewhat know whats going here on based on previous discussions about this PR, but without that prior context this being commented out is pretty confusing and I don't think the comment above provides enough explanation. Perhaps add more context in the comment or just remove this snippet entirely?
💬 josibake commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1598126028)
nit: might be worth adding a comment here explaining this. I was also surprised to see this not returning `false` and couldn't understand why this was different from other methods until I read your comment here.
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1598126028)
nit: might be worth adding a comment here explaining this. I was also surprised to see this not returning `false` and couldn't understand why this was different from other methods until I read your comment here.