💬 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.
💬 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_r1163319528)
I left them without quotes as none of the other category values in the -debug and -debugexclude helps are in quotes. But you may be right that it's nevertheless clearer to add them here. Done.
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1163319528)
I left them without quotes as none of the other category values in the -debug and -debugexclude helps are in quotes. But you may be right that it's nevertheless clearer to add them here. Done.
💬 mzumsande commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1504081199)
> I am not sure about CJDNS. Normally I would say treat it like Tor and I2P (but I even had the heretic idea to do that with IPv4 and IPv6 too, so maybe I lean a bit to the paranoid side, don't listen to me too much). However there are very few CJDNS addresses right now on mainnet and if we only advertise our CJDNS address to CJDNS peers that could impede its adoption.
Makes sense to me - if bitcoin over CJDNS gets more popular we could revisit this later.
> Ultimately, the perfect soluti
...
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1504081199)
> I am not sure about CJDNS. Normally I would say treat it like Tor and I2P (but I even had the heretic idea to do that with IPv4 and IPv6 too, so maybe I lean a bit to the paranoid side, don't listen to me too much). However there are very few CJDNS addresses right now on mainnet and if we only advertise our CJDNS address to CJDNS peers that could impede its adoption.
Makes sense to me - if bitcoin over CJDNS gets more popular we could revisit this later.
> Ultimately, the perfect soluti
...
📝 theStack opened a pull request: "doc: update OpenBSD build docs for 7.3 (external signer support available)"
(https://github.com/bitcoin/bitcoin/pull/27449)
With OpenBSD 7.3, the waitid(2) system call is implemented (see https://github.com/openbsd/src/commit/8112871f19bbd25e86c93d0f901071ca2335a352, first mentioned kernel improvement at https://www.openbsd.org/73.html).
This means Boost.Process finally doesn't fail to compile anymore and we can remove the build hint about missing external signer support. Tested on my amd64 machine by reconfiguring / rebuilding master branch and successfully running the functional test wallet_signer.py. :heavy_che
...
(https://github.com/bitcoin/bitcoin/pull/27449)
With OpenBSD 7.3, the waitid(2) system call is implemented (see https://github.com/openbsd/src/commit/8112871f19bbd25e86c93d0f901071ca2335a352, first mentioned kernel improvement at https://www.openbsd.org/73.html).
This means Boost.Process finally doesn't fail to compile anymore and we can remove the build hint about missing external signer support. Tested on my amd64 machine by reconfiguring / rebuilding master branch and successfully running the functional test wallet_signer.py. :heavy_che
...
💬 rsafier commented on pull request "Allow configuirng target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1504092196)
Concept ACK. Been wanting this for a while
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1504092196)
Concept ACK. Been wanting this for a while
💬 mzumsande commented on pull request "Allow configuirng target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163340252)
This fails the linter, could use `ParseInt64()`.
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163340252)
This fails the linter, could use `ParseInt64()`.
💬 mzumsande commented on pull request "Allow configuirng target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163341355)
Maybe require that `signetblocktime > 0` ? Probably bad things would happen otherwise.
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163341355)
Maybe require that `signetblocktime > 0` ? Probably bad things would happen otherwise.
💬 benthecarman commented on pull request "Allow configuirng target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163356429)
Did `signetblocktime >= 0` just to be sure
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163356429)
Did `signetblocktime >= 0` just to be sure
💬 ajtowns commented on pull request "mempool / miner: regularly flush <=0-fee entries and mine everything in the mempool":
(https://github.com/bitcoin/bitcoin/pull/27018#discussion_r1163371721)
From the ephemeral anchors review club:
> `<glozow> ok hm. i kinda need to get rid of blockmintxfee for pakcage relay tho`
Can you elaborate on that? You can just drop the blockminfee commit from this PR, and it seems fine, no?
The only result of setting `-blockminfee` is that if its value is higher than `-minrelaytxfee` you'll potentially never empty your own memory even if you're the only miner and no new transactions are being created/relayed. That's undesirable as default behaviour,
...
(https://github.com/bitcoin/bitcoin/pull/27018#discussion_r1163371721)
From the ephemeral anchors review club:
> `<glozow> ok hm. i kinda need to get rid of blockmintxfee for pakcage relay tho`
Can you elaborate on that? You can just drop the blockminfee commit from this PR, and it seems fine, no?
The only result of setting `-blockminfee` is that if its value is higher than `-minrelaytxfee` you'll potentially never empty your own memory even if you're the only miner and no new transactions are being created/relayed. That's undesirable as default behaviour,
...
💬 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_r1163430354)
Yes, I've changed `GetUpdatedStateFn` to `TryUpdatingStateFn`, and `get_updated_state` to `try_updating_state`.
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1163430354)
Yes, I've changed `GetUpdatedStateFn` to `TryUpdatingStateFn`, and `get_updated_state` to `try_updating_state`.