💬 jamesob commented on pull request "logging, net: add ASN from peers on logs":
(https://github.com/bitcoin/bitcoin/pull/27412#issuecomment-1503832004)
ACK 0076bed45eb2b42111fa3f4c95181393c685a42e ([`jamesob/ackr/27412.1.brunoerg.logging_net_add_asn_from`](https://github.com/jamesob/bitcoin/tree/ackr/27412.1.brunoerg.logging_net_add_asn_from))
Reviewed code, built locally. Change looks good. Going to test with -asmap today.
<details><summary>Show signature data</summary>
<p>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK 0076bed45eb2b42111fa3f4c95181393c685a42e ([`jamesob/ackr/27412.1.brunoerg.logging_net_add_asn_from`
...
(https://github.com/bitcoin/bitcoin/pull/27412#issuecomment-1503832004)
ACK 0076bed45eb2b42111fa3f4c95181393c685a42e ([`jamesob/ackr/27412.1.brunoerg.logging_net_add_asn_from`](https://github.com/jamesob/bitcoin/tree/ackr/27412.1.brunoerg.logging_net_add_asn_from))
Reviewed code, built locally. Change looks good. Going to test with -asmap today.
<details><summary>Show signature data</summary>
<p>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK 0076bed45eb2b42111fa3f4c95181393c685a42e ([`jamesob/ackr/27412.1.brunoerg.logging_net_add_asn_from`
...
👍 1440000bytes approved a pull request: "Allow configuirng target block time for a signet"
(https://github.com/bitcoin/bitcoin/pull/27446#pullrequestreview-1379793799)
utACK https://github.com/bitcoin/bitcoin/pull/27446/commits/15ca24bb49cd6a5027dcf57fc7b9bd1d33d367db
(https://github.com/bitcoin/bitcoin/pull/27446#pullrequestreview-1379793799)
utACK https://github.com/bitcoin/bitcoin/pull/27446/commits/15ca24bb49cd6a5027dcf57fc7b9bd1d33d367db
💬 earonesty commented on pull request "Allow configuirng target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1503852579)
concept ack. signet is for tinkering
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1503852579)
concept ack. signet is for tinkering
✅ achow101 closed an issue: "Selecting two coins will abort with "The amount exceeds your balance.""
(https://github.com/bitcoin-core/gui/issues/688)
(https://github.com/bitcoin-core/gui/issues/688)
🚀 achow101 merged a pull request: "wallet, gui: bugfix, getAvailableBalance skips selected coins"
(https://github.com/bitcoin/bitcoin/pull/26699)
(https://github.com/bitcoin/bitcoin/pull/26699)
💬 furszy commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1163166007)
It was because I was accessing the parent to click on the message box from the unit test (the dialog `exec()` method blocks the thread until the user presses any of the buttons). And, later on, changed my mind and accessed it via the `QApplication::topLevelWidgets` call.
It doesn't hurt to have it. This would had been a leak if the message box would had been constructed via a `new` call.
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1163166007)
It was because I was accessing the parent to click on the message box from the unit test (the dialog `exec()` method blocks the thread until the user presses any of the buttons). And, later on, changed my mind and accessed it via the `QApplication::topLevelWidgets` call.
It doesn't hurt to have it. This would had been a leak if the message box would had been constructed via a `new` call.
💬 furszy commented on issue "Unable to create PSBT for legacy watchonly wallets in the GUI":
(https://github.com/bitcoin/bitcoin/issues/26687#issuecomment-1503887860)
https://github.com/bitcoin/bitcoin/pull/26699 merged, can be closed.
(https://github.com/bitcoin/bitcoin/issues/26687#issuecomment-1503887860)
https://github.com/bitcoin/bitcoin/pull/26699 merged, can be closed.
💬 sdaftuar commented on pull request "mempool: disallow txns under min relay fee, even in packages":
(https://github.com/bitcoin/bitcoin/pull/26933#discussion_r1162786364)
Not sure if I'm just reading too fast, but was the intent here to check that the mempool minfee for the *child* is <= 800? Seems like we did the parent checks up above, but maybe I'm missing something...?
(https://github.com/bitcoin/bitcoin/pull/26933#discussion_r1162786364)
Not sure if I'm just reading too fast, but was the intent here to check that the mempool minfee for the *child* is <= 800? Seems like we did the parent checks up above, but maybe I'm missing something...?
✅ fanquake closed an issue: "Unable to create PSBT for legacy watchonly wallets in the GUI"
(https://github.com/bitcoin/bitcoin/issues/26687)
(https://github.com/bitcoin/bitcoin/issues/26687)
💬 ishaanam commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1163192633)
Done
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1163192633)
Done
💬 ishaanam commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1163193445)
Right, I've made them more generic now.
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1163193445)
Right, I've made them more generic now.
💬 achow101 commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1163208401)
I agree that this option seems pointless. It's never used currently, and I don't think it makes sense to ever use it in the tests unless specifically testing immature transactions, in which case the utxo should be known explicitly by the caller.
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1163208401)
I agree that this option seems pointless. It's never used currently, and I don't think it makes sense to ever use it in the tests unless specifically testing immature transactions, in which case the utxo should be known explicitly by the caller.
💬 furszy commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1163196400)
No need to re-acquire `cs_wallet` here (`RecursiveUpdateTxState` already locks `cs_wallet`) . Can add the exclude lock annotation to the function instead:
```c++
const auto& try_update_state = [&](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
if (conflictconfirms >= GetTxDepthInMainChain(wtx)) return false;
wtx.m_state = TxStateConflicted{hashBlock, conflicting_height};
return true;
};
```
(same for the others)
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1163196400)
No need to re-acquire `cs_wallet` here (`RecursiveUpdateTxState` already locks `cs_wallet`) . Can add the exclude lock annotation to the function instead:
```c++
const auto& try_update_state = [&](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
if (conflictconfirms >= GetTxDepthInMainChain(wtx)) return false;
wtx.m_state = TxStateConflicted{hashBlock, conflicting_height};
return true;
};
```
(same for the others)
💬 furszy commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1163192581)
The `GetUpdatedStateFn` function isn't really a getter, what about calling it `TryUpdateState`?.
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1163192581)
The `GetUpdatedStateFn` function isn't really a getter, what about calling it `TryUpdateState`?.
💬 Xekyo commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#issuecomment-1503974799)
reACK https://github.com/bitcoin/bitcoin/commit/635c50a57b3f72f83f1ee681f6a4b7766b8988f6
but appears to need rebase
(https://github.com/bitcoin/bitcoin/pull/27217#issuecomment-1503974799)
reACK https://github.com/bitcoin/bitcoin/commit/635c50a57b3f72f83f1ee681f6a4b7766b8988f6
but appears to need rebase
💬 achow101 commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1163240135)
The MiniWallet does not update the confirmation counts of its UTXOs when new blocks are generated. When the blocks are generated with a different generator, it will still think that its generated coins are immature even if enough blocks have been mined to make them mature. When this behavior is coupled with the change to prefer matured coins, the MiniWallet ends up later having a very restricted UTXO set as it will have a few mature coins that are the result of making transaction chains. This oc
...
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1163240135)
The MiniWallet does not update the confirmation counts of its UTXOs when new blocks are generated. When the blocks are generated with a different generator, it will still think that its generated coins are immature even if enough blocks have been mined to make them mature. When this behavior is coupled with the change to prefer matured coins, the MiniWallet ends up later having a very restricted UTXO set as it will have a few mature coins that are the result of making transaction chains. This oc
...
💬 achow101 commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1163243925)
In 894b2531991f8179022c158c2c15c06f0b774f0e "test: fix intermittent issue in `feature_bip68_sequence`"
The confirmation counts for the UTXOs can be very outdated when blocks are generated by things other than the MiniWallet. It would be nice if these could be kept up to date so that this filter is accurate.
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1163243925)
In 894b2531991f8179022c158c2c15c06f0b774f0e "test: fix intermittent issue in `feature_bip68_sequence`"
The confirmation counts for the UTXOs can be very outdated when blocks are generated by things other than the MiniWallet. It would be nice if these could be kept up to date so that this filter is accurate.
💬 jonatack commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1163319014)
Looking again (thanks!), both of these `NONE` entries can be dropped. Updated to add prior test coverage to spec affected behavior, then removed them.
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1163319014)
Looking again (thanks!), both of these `NONE` entries can be dropped. Updated to add prior test coverage to spec affected behavior, then removed them.
💬 jonatack commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1163319182)
Thanks for making me re-verify. This case will never be hit and is only present to appease the compiler (`error: enumeration value 'NONE' not handled in switch`), so leaving it as an empty string, added a comment, and moved it to the last case in the switch.
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1163319182)
Thanks for making me re-verify. This case will never be hit and is only present to appease the compiler (`error: enumeration value 'NONE' not handled in switch`), so leaving it as an empty string, added a comment, and moved it to the last case in the switch.
💬 jonatack commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1163319415)
This commit is to have the same behavior (and code) for the -debug and -debugexclude config options. I'm not sure what your suggestion is, but I'm happy to look at a working example.
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1163319415)
This commit is to have the same behavior (and code) for the -debug and -debugexclude config options. I'm not sure what your suggestion is, but I'm happy to look at a working example.