💬 tcharding commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765886440)
In 7d46c2488679ccca70cdd1dc71100724347a47e8 this is a pretty big change behavour, right? I would have expected more in the patch that just a one line change (eg docs somewhere or perhaps a test somewhere that checks default behavour).
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765886440)
In 7d46c2488679ccca70cdd1dc71100724347a47e8 this is a pretty big change behavour, right? I would have expected more in the patch that just a one line change (eg docs somewhere or perhaps a test somewhere that checks default behavour).
💬 tcharding commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765883458)
Whitespace is different here compared to below (`mtx, /* version=*/ 2`). I don't know if you guys worry about that sort of thing.
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765883458)
Whitespace is different here compared to below (`mtx, /* version=*/ 2`). I don't know if you guys worry about that sort of thing.
💬 tcharding commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765869737)
Is there a reason to compute the locktime again? It is done in the call to `GetUnsignedTx` just above.
(In 0cf1a9febb0205dadc03cd87a7af074e7b4cfd8c)
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765869737)
Is there a reason to compute the locktime again? It is done in the call to `GetUnsignedTx` just above.
(In 0cf1a9febb0205dadc03cd87a7af074e7b4cfd8c)
💬 tcharding commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765876840)
Why change to a signed int? IIUC this is checked against the version field in transaction which is unsigned.
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765876840)
Why change to a signed int? IIUC this is checked against the version field in transaction which is unsigned.
💬 Ajeetprajapati315 commented on something "":
(https://github.com/bitcoin/bitcoin/commit/e49d858aab9bfae2455d87226d9ebebbdf2c40c8#commitcomment-146902742)
crypto.com
8700755231
(https://github.com/bitcoin/bitcoin/commit/e49d858aab9bfae2455d87226d9ebebbdf2c40c8#commitcomment-146902742)
crypto.com
8700755231
💬 Ajeetprajapati315 commented on something "":
(https://github.com/bitcoin/bitcoin/commit/e49d858aab9bfae2455d87226d9ebebbdf2c40c8#commitcomment-146902756)
BTC
(https://github.com/bitcoin/bitcoin/commit/e49d858aab9bfae2455d87226d9ebebbdf2c40c8#commitcomment-146902756)
BTC
💬 achow101 commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765908400)
Good point, removed.
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765908400)
Good point, removed.
💬 achow101 commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765908620)
It was unsigned when this PR was written 3 years ago lol.
Updated to be unsigned.
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765908620)
It was unsigned when this PR was written 3 years ago lol.
Updated to be unsigned.
💬 achow101 commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765908738)
Fixed the whitespace to be consistent with our style guidelines
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765908738)
Fixed the whitespace to be consistent with our style guidelines
💬 achow101 commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765909311)
I've added a basic test for this and a release note fragment. The bulk of the test changes in previous commits is to setup for this though.
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765909311)
I've added a basic test for this and a release note fragment. The bulk of the test changes in previous commits is to setup for this though.
💬 tcharding commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765924789)
Cheers
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765924789)
Cheers
💬 tcharding commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765925066)
Nice!
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765925066)
Nice!
💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#issuecomment-2359737551)
re-ACK 4feaa28728442b0fd29a677d2b170a05fdf967c0
(https://github.com/bitcoin/bitcoin/pull/30849#issuecomment-2359737551)
re-ACK 4feaa28728442b0fd29a677d2b170a05fdf967c0
💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1765988081)
I meant that the `HaveCoin` signature is not relevant to this PR. So, just modifying it by moving the brace to the next line and moving the `&` to be next to the type is making a style change to a line that doesn't need to be touched in this PR.
If a line must be changed for the PR anyways, then yes I agree the style of the entire line should be updated.
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1765988081)
I meant that the `HaveCoin` signature is not relevant to this PR. So, just modifying it by moving the brace to the next line and moving the `&` to be next to the type is making a style change to a line that doesn't need to be touched in this PR.
If a line must be changed for the PR anyways, then yes I agree the style of the entire line should be updated.
💬 furszy commented on pull request "Fix linking when configured with `-DENABLE_WALLET=OFF` (Qt 6)":
(https://github.com/bitcoin-core/gui/pull/837#discussion_r1766029532)
How the `RPCExecutor::request` method compiles when this line doesn't exist?
(https://github.com/bitcoin-core/gui/pull/837#discussion_r1766029532)
How the `RPCExecutor::request` method compiles when this line doesn't exist?
👍 tdb3 approved a pull request: "rpc: add getdescriptoractivity"
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2314225218)
ACK 1365ee8e9c7a20aa63bcddb1a6d5843c05ff9330
This is a value-add that I hope gets more traction/review.
Left a few more minor nits.
As time allows, I may exercise additional descriptors.
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2314225218)
ACK 1365ee8e9c7a20aa63bcddb1a6d5843c05ff9330
This is a value-add that I hope gets more traction/review.
Left a few more minor nits.
As time allows, I may exercise additional descriptors.
💬 tdb3 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1766056564)
If this gets touched again, could add asserts to check that the unconfirmed tx has blank blockhash and -1 height. Could be left for a follow-up PR.
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1766056564)
If this gets touched again, could add asserts to check that the unconfirmed tx has blank blockhash and -1 height. Could be left for a follow-up PR.
💬 tdb3 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1766035266)
nit: if another update occurs, may want to use `STR_HEX` for `scriptpubkey_hex`
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1766035266)
nit: if another update occurs, may want to use `STR_HEX` for `scriptpubkey_hex`
✅ LuizWT closed a pull request: "Optimize: convert trusted keys list to a set for better performance"
(https://github.com/bitcoin/bitcoin/pull/30925)
(https://github.com/bitcoin/bitcoin/pull/30925)
💬 LuizWT commented on pull request "Optimize: convert trusted keys list to a set for better performance":
(https://github.com/bitcoin/bitcoin/pull/30925#discussion_r1766112838)
> This is not even an equivalent expression. When you convert a list to a set, the order of the lines is lost. Therefore, accessing the first element with [0] could give you any of the lines randomly, not necessarily the first line from the original file. In this case you needed index 0 ( root ).
You're right. It makes no sense to use a set in this context. Thanks for pointing this out
(https://github.com/bitcoin/bitcoin/pull/30925#discussion_r1766112838)
> This is not even an equivalent expression. When you convert a list to a set, the order of the lines is lost. Therefore, accessing the first element with [0] could give you any of the lines randomly, not necessarily the first line from the original file. In this case you needed index 0 ( root ).
You're right. It makes no sense to use a set in this context. Thanks for pointing this out