💬 ishaanam commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#discussion_r1414651744)
Done
(https://github.com/bitcoin/bitcoin/pull/28944#discussion_r1414651744)
Done
💬 ishaanam commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#discussion_r1414652209)
Done
(https://github.com/bitcoin/bitcoin/pull/28944#discussion_r1414652209)
Done
💬 luke-jr commented on pull request "Fix SSE4.1-related issues":
(https://github.com/bitcoin/bitcoin/pull/28893#issuecomment-1839830013)
Weak Concept NACK: We should probably still build the SSE4.1 code if the compiler supports it. The flag should only affect the rest of the codebase, so that the software will run on a system without SSE4.1 (but still use the optimised crypto on systems that have it).
(https://github.com/bitcoin/bitcoin/pull/28893#issuecomment-1839830013)
Weak Concept NACK: We should probably still build the SSE4.1 code if the compiler supports it. The flag should only affect the rest of the codebase, so that the software will run on a system without SSE4.1 (but still use the optimised crypto on systems that have it).
💬 luke-jr 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-1839831023)
This is fixed by #13789 which was inexplicably closed for no reason
(https://github.com/bitcoin/bitcoin/issues/28864#issuecomment-1839831023)
This is fixed by #13789 which was inexplicably closed for no reason
💬 luke-jr commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1414735112)
Shouldn't this be `NO_KEY_TIME` explicitly?
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1414735112)
Shouldn't this be `NO_KEY_TIME` explicitly?
💬 luke-jr commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1414735331)
Seems like `NO_KEY_TIME` ought to be `0` so it triggers a full scan
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1414735331)
Seems like `NO_KEY_TIME` ought to be `0` so it triggers a full scan
💬 luke-jr commented on pull request "script/sign: avoid duplicated signature verification after signing (+introduce signing benchmarks)":
(https://github.com/bitcoin/bitcoin/pull/28923#issuecomment-1839848382)
Does this reduce our safety against memory corruption or similar?
(https://github.com/bitcoin/bitcoin/pull/28923#issuecomment-1839848382)
Does this reduce our safety against memory corruption or similar?
💬 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."