💬 luke-jr commented on pull request "wallet: Add scan_utxo option to getbalances RPC":
(https://github.com/bitcoin/bitcoin/pull/28930#issuecomment-1839854997)
getbalances seems like the wrong place for this?
(https://github.com/bitcoin/bitcoin/pull/28930#issuecomment-1839854997)
getbalances seems like the wrong place for this?
💬 techy2 commented on issue "getrawtransaction xxxxxx.... 2 causes a segfault":
(https://github.com/bitcoin/bitcoin/issues/28986#issuecomment-1839879785)
computer is an HP Proliant GL 380 G7 that has been up for over a year, it is very reliable and runs many other tasks. Don't think it is a hardware issue or I would think there would be other problems as well and there are not.
-tindex is off
The transaction is not one in my wallet, just a random tx from the network I picked off the block explorer, I was doing some testing and picked it just to have a look at the transaction structure as reported by the daemon. In hindsight, because it was
...
(https://github.com/bitcoin/bitcoin/issues/28986#issuecomment-1839879785)
computer is an HP Proliant GL 380 G7 that has been up for over a year, it is very reliable and runs many other tasks. Don't think it is a hardware issue or I would think there would be other problems as well and there are not.
-tindex is off
The transaction is not one in my wallet, just a random tx from the network I picked off the block explorer, I was doing some testing and picked it just to have a look at the transaction structure as reported by the daemon. In hindsight, because it was
...
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1414758537)
Only 2 of the 6 previous `flags` modifications used copy assignment, the other 4 use OR assignment. The 2 that use copy assignment we know logically that they were just created and therefore their flags are 0 and we are not overwriting anything. Therefore an OR assignment in those 2 cases is logically equivalent.
So if we were to go with your second suggestion we would need to modify behavior to make sure we assign the new flags we wish to set OR'd with the current flags. I think that would mak
...
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1414758537)
Only 2 of the 6 previous `flags` modifications used copy assignment, the other 4 use OR assignment. The 2 that use copy assignment we know logically that they were just created and therefore their flags are 0 and we are not overwriting anything. Therefore an OR assignment in those 2 cases is logically equivalent.
So if we were to go with your second suggestion we would need to modify behavior to make sure we assign the new flags we wish to set OR'd with the current flags. I think that would mak
...
🤔 pablomartin4btc reviewed a pull request: "Fee Estimator updates from Validation Interface/CScheduler thread"
(https://github.com/bitcoin/bitcoin/pull/28368#pullrequestreview-1763789905)
post merge tACK 91504cbe0de2b74ef1aa2709761aaf0597ec66a2
Tested manually using `RPC` commands `estimatesmartfee` and `estimaterawfee` (following cases from `/test/functional/feature_fee_estimation.py`).
(https://github.com/bitcoin/bitcoin/pull/28368#pullrequestreview-1763789905)
post merge tACK 91504cbe0de2b74ef1aa2709761aaf0597ec66a2
Tested manually using `RPC` commands `estimatesmartfee` and `estimaterawfee` (following cases from `/test/functional/feature_fee_estimation.py`).
💬 luke-jr commented on pull request "index: block filters sync, reduce disk read operations by caching last header":
(https://github.com/bitcoin/bitcoin/pull/28955#issuecomment-1839900641)
>Probably, it is because the benchmarks, same as the unit tests, create temp directories. Which may be faster to access than regular directories.
Is it, though? I would think the OS should have this cached regardless?
(https://github.com/bitcoin/bitcoin/pull/28955#issuecomment-1839900641)
>Probably, it is because the benchmarks, same as the unit tests, create temp directories. Which may be faster to access than regular directories.
Is it, though? I would think the OS should have this cached regardless?
🤔 luke-jr requested changes to a pull request: "build: disable external-signer for Windows"
(https://github.com/bitcoin/bitcoin/pull/28967#pullrequestreview-1763820931)
Let's at least let builders explicitly enable it?
(https://github.com/bitcoin/bitcoin/pull/28967#pullrequestreview-1763820931)
Let's at least let builders explicitly enable it?
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-1839920097)
@martinus thank you very much for your review! I updated the PR with all your suggestions.
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-1839920097)
@martinus thank you very much for your review! I updated the PR with all your suggestions.
💬 luke-jr commented on pull request "doc: explain what the wallet password does":
(https://github.com/bitcoin/bitcoin/pull/28974#discussion_r1414811800)
There is no seed phrase.
(https://github.com/bitcoin/bitcoin/pull/28974#discussion_r1414811800)
There is no seed phrase.
💬 luke-jr commented on pull request "doc: explain what the wallet password does":
(https://github.com/bitcoin/bitcoin/pull/28974#discussion_r1414812287)
Someone with such access can just install a keylogger.
(https://github.com/bitcoin/bitcoin/pull/28974#discussion_r1414812287)
Someone with such access can just install a keylogger.
💬 luke-jr commented on pull request "doc: explain what the wallet password does":
(https://github.com/bitcoin/bitcoin/pull/28974#discussion_r1414810714)
No need to say "introduced in" especially since v0.4.0 was not Bitcoin Core in any meaningful way
(https://github.com/bitcoin/bitcoin/pull/28974#discussion_r1414810714)
No need to say "introduced in" especially since v0.4.0 was not Bitcoin Core in any meaningful way
💬 luke-jr commented on pull request "rpc: encryptwallet help, mention HD seed rotation and backup requirement":
(https://github.com/bitcoin/bitcoin/pull/28980#issuecomment-1839933366)
utACK
(https://github.com/bitcoin/bitcoin/pull/28980#issuecomment-1839933366)
utACK
🤔 luke-jr requested changes to a pull request: "Replace Boost.Process with cpp-subprocess"
(https://github.com/bitcoin/bitcoin/pull/28981#pullrequestreview-1763876066)
>>Maybe a subtree from our own fork?
>I think that would mostly just re-introduce the awkwardness we ended up with with univalue, for not much benefit, and for code that shouldn't be critical (i.e compared to leveldb & libsecp256k1), this is for an optional feature, that is not used by a large proportion of the people running this software.
Which means it should be an entirely external dependency we just check for in configure...
The only thing worse than a subtree, is entirely vendorin
...
(https://github.com/bitcoin/bitcoin/pull/28981#pullrequestreview-1763876066)
>>Maybe a subtree from our own fork?
>I think that would mostly just re-introduce the awkwardness we ended up with with univalue, for not much benefit, and for code that shouldn't be critical (i.e compared to leveldb & libsecp256k1), this is for an optional feature, that is not used by a large proportion of the people running this software.
Which means it should be an entirely external dependency we just check for in configure...
The only thing worse than a subtree, is entirely vendorin
...
💬 jonatack commented on pull request "rpc: encryptwallet help, mention HD seed rotation and backup requirement":
(https://github.com/bitcoin/bitcoin/pull/28980#discussion_r1414836494)
This is now a bit verbose and redundant, and does not address https://github.com/bitcoin/bitcoin/pull/28980#discussion_r1414581640.
Suggest:
"Therefore, it is crucial to securely back up the newly generated wallet file using the `backupwallet` RPC."
(https://github.com/bitcoin/bitcoin/pull/28980#discussion_r1414836494)
This is now a bit verbose and redundant, and does not address https://github.com/bitcoin/bitcoin/pull/28980#discussion_r1414581640.
Suggest:
"Therefore, it is crucial to securely back up the newly generated wallet file using the `backupwallet` RPC."
💬 martinus commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-1840061587)
I didn't think this through yet, but this might be an alternative way to achieve the same thing: Instead of the double linked list, split the `cacheCoins` map up into 4 different maps, one map for each flag setting, e.g. as a `std::array<CCoinMap, 4> m_cache_coins_per_flag;`. Index is the flag, e.g. `m_cache_coins_per_flag[FRESH | DIRTY]` to get the map with all entries that are fresh & dirty. Instead of setting a flag & linking the list, use the unordered_map's `auto node = extract()` and `ins
...
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-1840061587)
I didn't think this through yet, but this might be an alternative way to achieve the same thing: Instead of the double linked list, split the `cacheCoins` map up into 4 different maps, one map for each flag setting, e.g. as a `std::array<CCoinMap, 4> m_cache_coins_per_flag;`. Index is the flag, e.g. `m_cache_coins_per_flag[FRESH | DIRTY]` to get the map with all entries that are fresh & dirty. Instead of setting a flag & linking the list, use the unordered_map's `auto node = extract()` and `ins
...
💬 maflcko commented on issue "build: Configuring with `-mno-sse4.1` does not fail the sse4.1 instrinsics check":
(https://github.com/bitcoin/bitcoin/issues/28864#issuecomment-1840181002)
> This is fixed by #13789 which was inexplicably closed for no reason
It was closed due to https://github.com/bitcoin/bitcoin/issues/13758#issuecomment-1216319878
(https://github.com/bitcoin/bitcoin/issues/28864#issuecomment-1840181002)
> This is fixed by #13789 which was inexplicably closed for no reason
It was closed due to https://github.com/bitcoin/bitcoin/issues/13758#issuecomment-1216319878
💬 maflcko commented on pull request "crypto/sha256: Use pragmas to enforce necessary intrinsics for GCC and Clang":
(https://github.com/bitcoin/bitcoin/pull/13789#issuecomment-1840184719)
(This was closed as part of commit 02aefa169a9e6ed12c7bd8f3392adcd073d8d56b)
(https://github.com/bitcoin/bitcoin/pull/13789#issuecomment-1840184719)
(This was closed as part of commit 02aefa169a9e6ed12c7bd8f3392adcd073d8d56b)
💬 maflcko commented on issue "getrawtransaction xxxxxx.... 2 causes a segfault":
(https://github.com/bitcoin/bitcoin/issues/28986#issuecomment-1840199422)
> when "verbosity" = 2, that allows the code to fall all the way to TxToJSON with a bad pointer in hash_block where it will not do the right thing.
hash_block is not a pointer. If it was unset, but read, it would just be all-zero.
> After restarting the daemon and since then including now, the rpc command correctly tells me that:
> "No such mempool transaction. Use -txindex or provide a block hash to enable blockchain transaction queries. Use gettransaction for wallet transactions."
...
(https://github.com/bitcoin/bitcoin/issues/28986#issuecomment-1840199422)
> when "verbosity" = 2, that allows the code to fall all the way to TxToJSON with a bad pointer in hash_block where it will not do the right thing.
hash_block is not a pointer. If it was unset, but read, it would just be all-zero.
> After restarting the daemon and since then including now, the rpc command correctly tells me that:
> "No such mempool transaction. Use -txindex or provide a block hash to enable blockchain transaction queries. Use gettransaction for wallet transactions."
...
🤔 S3RK reviewed a pull request: "wallet: Have the wallet store the key for automatically generated descriptors"
(https://github.com/bitcoin/bitcoin/pull/26728#pullrequestreview-1756870858)
Code review ACK ac3b860bff4279ff470560d0ccad9b2fc09672a9
I carefully re-reviewed the new approach. Looks good to me. Going to repeat the tests now.
(https://github.com/bitcoin/bitcoin/pull/26728#pullrequestreview-1756870858)
Code review ACK ac3b860bff4279ff470560d0ccad9b2fc09672a9
I carefully re-reviewed the new approach. Looks good to me. Going to repeat the tests now.
💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1413520408)
in 14738e9a12c3d128f52875cf56b2ecacc6e62642
Can we make `key` optional to avoid passing invalid keys?
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1413520408)
in 14738e9a12c3d128f52875cf56b2ecacc6e62642
Can we make `key` optional to avoid passing invalid keys?
💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1413528877)
in eca964a8c07cf43b45e2c0482614ded390d42fec
nit: Could also `Assume` that decryption is successful
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1413528877)
in eca964a8c07cf43b45e2c0482614ded390d42fec
nit: Could also `Assume` that decryption is successful