💬 furszy commented on pull request "index: move disk read lookups to base class":
(https://github.com/bitcoin/bitcoin/pull/32694#issuecomment-2960008481)
Thanks for the review ryanofsky and TheCharlatan!
Updated per feedback.
> Can you double-check the commit message of https://github.com/bitcoin/bitcoin/commit/50696f07784b5dcf04a398f7592dead4dde6d2e9. I think there is a word too much in the first and second sentence.
Done.
(https://github.com/bitcoin/bitcoin/pull/32694#issuecomment-2960008481)
Thanks for the review ryanofsky and TheCharlatan!
Updated per feedback.
> Can you double-check the commit message of https://github.com/bitcoin/bitcoin/commit/50696f07784b5dcf04a398f7592dead4dde6d2e9. I think there is a word too much in the first and second sentence.
Done.
💬 pinheadmz commented on pull request "BIP-348 (OP_CHECKSIGFROMSTACK) (regtest only)":
(https://github.com/bitcoin/bitcoin/pull/32247#issuecomment-2960013289)
In an effort to keep pull requests focused on technical discussion, I invite all contributors of conceptual review to post here:
https://github.com/bitcoin-core/meta/discussions/28
Please try to keep pull request comments focused on the code changes, and move all other comments including "concept N/ACK" to the discussion page.
(https://github.com/bitcoin/bitcoin/pull/32247#issuecomment-2960013289)
In an effort to keep pull requests focused on technical discussion, I invite all contributors of conceptual review to post here:
https://github.com/bitcoin-core/meta/discussions/28
Please try to keep pull request comments focused on the code changes, and move all other comments including "concept N/ACK" to the discussion page.
💬 pinheadmz commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (regtest only)":
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2960013769)
In an effort to keep pull requests focused on technical discussion, I invite all contributors of conceptual review to post here:
https://github.com/bitcoin-core/meta/discussions/28
Please try to keep pull request comments focused on the code changes, and move all other comments including "concept N/ACK" to the discussion page.
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2960013769)
In an effort to keep pull requests focused on technical discussion, I invite all contributors of conceptual review to post here:
https://github.com/bitcoin-core/meta/discussions/28
Please try to keep pull request comments focused on the code changes, and move all other comments including "concept N/ACK" to the discussion page.
💬 pablomartin4btc commented on pull request "wallet, refactor: Remove Legacy wallet unused warnings and errors":
(https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2138416600)
Good catch! Done, thanks.
(https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2138416600)
Good catch! Done, thanks.
💬 pablomartin4btc commented on pull request "wallet, refactor: Remove Legacy wallet unused warnings and errors":
(https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2138418436)
If was intentional but it makes sense. Done, thanks!
(https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2138418436)
If was intentional but it makes sense. Done, thanks!
💬 achow101 commented on issue "Internal bug detected: Fee needed > fee paid":
(https://github.com/bitcoin/bitcoin/issues/32683#issuecomment-2960123379)
> Should you be allowed to create a transaction on a watch only wallet?
Yes, that's pretty much the point of a watch-only wallet: to create unsigned transactions that the offline wallet can sign.
(https://github.com/bitcoin/bitcoin/issues/32683#issuecomment-2960123379)
> Should you be allowed to create a transaction on a watch only wallet?
Yes, that's pretty much the point of a watch-only wallet: to create unsigned transactions that the offline wallet can sign.
💬 achow101 commented on pull request "wallet, refactor: Remove Legacy wallet unused warnings and errors":
(https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2138470337)
In 8fbb1c9657ef034aa179655a0869db45be2aaac8 "wallet, refactor: Remove Legacy warnings and errors"
That's wrong and causing a CI failure. I think this check is unneeded.
(https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2138470337)
In 8fbb1c9657ef034aa179655a0869db45be2aaac8 "wallet, refactor: Remove Legacy warnings and errors"
That's wrong and causing a CI failure. I think this check is unneeded.
💬 achow101 commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#issuecomment-2960153127)
reACK f6b782f3aad4a6bcf823a9a0fabb4418bca1eea1
(https://github.com/bitcoin/bitcoin/pull/31405#issuecomment-2960153127)
reACK f6b782f3aad4a6bcf823a9a0fabb4418bca1eea1
💬 TheCharlatan commented on pull request "index: move disk read lookups to base class":
(https://github.com/bitcoin/bitcoin/pull/32694#discussion_r2138480616)
Of course, not sure what tripped me up here :(
(https://github.com/bitcoin/bitcoin/pull/32694#discussion_r2138480616)
Of course, not sure what tripped me up here :(
📝 Muniru0 opened a pull request: "docs: add guidance on initialism capitalisation in PascalCase identifiers."
(https://github.com/bitcoin/bitcoin/pull/32718)
Fixes #32698
**Problem**
Inconsistent initialization of class,function, method identifiers and structs. Spread throughout the codebase is [screaming / acronym caps](https://gist.github.com/adashrod/66564c690906c9b774e77ddacbd06e1b).
**Solution**
* Add a rule in the developer notes to use consistent camelcase naming convention.
* A class names like `JSONRPCRequest` should be `JsonRpcRequest`.
all existing tests pass.
(https://github.com/bitcoin/bitcoin/pull/32718)
Fixes #32698
**Problem**
Inconsistent initialization of class,function, method identifiers and structs. Spread throughout the codebase is [screaming / acronym caps](https://gist.github.com/adashrod/66564c690906c9b774e77ddacbd06e1b).
**Solution**
* Add a rule in the developer notes to use consistent camelcase naming convention.
* A class names like `JSONRPCRequest` should be `JsonRpcRequest`.
all existing tests pass.
💬 achow101 commented on pull request "tests: Remove hardcoded addresstype in `rpc_psbt.py`":
(https://github.com/bitcoin/bitcoin/pull/32701#issuecomment-2960166699)
> if instead we should update the getnewaddress calls to include address_type where needed
I think `getnewaddress` calls should be explicit about address types when a specific type is needed.
(https://github.com/bitcoin/bitcoin/pull/32701#issuecomment-2960166699)
> if instead we should update the getnewaddress calls to include address_type where needed
I think `getnewaddress` calls should be explicit about address types when a specific type is needed.
💬 maflcko commented on pull request "wallet, refactor: Remove Legacy wallet unused warnings and errors":
(https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2138490743)
Yeah, the simplification of `a && !true` is `a && false` is `false`, so dead code and not `a`.
(https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2138490743)
Yeah, the simplification of `a && !true` is `a && false` is `false`, so dead code and not `a`.
💬 maflcko commented on pull request "docs: add guidance on initialism capitalisation in PascalCase identifiers.":
(https://github.com/bitcoin/bitcoin/pull/32718#issuecomment-2960168808)
> all existing tests pass.
no they do not
(https://github.com/bitcoin/bitcoin/pull/32718#issuecomment-2960168808)
> all existing tests pass.
no they do not
💬 BitcoinMechanic commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2960169750)
> I don't think people understand that this is _by default_. You can still just set your own limits in your config. If you disagree, you can just change it on your nodes.
>
> This was technically the correct thing to do, even if all the bots on X said otherwise. This whole thing should've been a big nothingburger. Most of you all commenting on the issue aren't understanding the effect of the changes at all. Mob behaviour towards devs should not be tolerated.
It's on Core to have sane defau
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2960169750)
> I don't think people understand that this is _by default_. You can still just set your own limits in your config. If you disagree, you can just change it on your nodes.
>
> This was technically the correct thing to do, even if all the bots on X said otherwise. This whole thing should've been a big nothingburger. Most of you all commenting on the issue aren't understanding the effect of the changes at all. Mob behaviour towards devs should not be tolerated.
It's on Core to have sane defau
...
✅ Muniru0 closed a pull request: "docs: add guidance on initialism capitalisation in PascalCase identifiers."
(https://github.com/bitcoin/bitcoin/pull/32718)
(https://github.com/bitcoin/bitcoin/pull/32718)
💬 achow101 commented on pull request "ci: Rewrite test-each-commit as py script":
(https://github.com/bitcoin/bitcoin/pull/32680#issuecomment-2960210974)
ACK fa9cfdf3be754b01e6ce73a0cc2f998b81e12970
(https://github.com/bitcoin/bitcoin/pull/32680#issuecomment-2960210974)
ACK fa9cfdf3be754b01e6ce73a0cc2f998b81e12970
💬 Muniru0 commented on pull request "docs: add guidance on initialism capitalisation in PascalCase identifiers.":
(https://github.com/bitcoin/bitcoin/pull/32718#issuecomment-2960212133)
> > all existing tests pass.
>
> no they do not
>
> (There is no point in trying to claim the tests pass. Most tests are run in the CI, so anyone can see for themselves or run them themselves. If there is a test that isn't run in CI, it would be good to mention it. However, this is just a doc change.)
I first push it to my branch and had no conflicts. So that is why I said the test passed before I push them here.
(https://github.com/bitcoin/bitcoin/pull/32718#issuecomment-2960212133)
> > all existing tests pass.
>
> no they do not
>
> (There is no point in trying to claim the tests pass. Most tests are run in the CI, so anyone can see for themselves or run them themselves. If there is a test that isn't run in CI, it would be good to mention it. However, this is just a doc change.)
I first push it to my branch and had no conflicts. So that is why I said the test passed before I push them here.
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138025794)
In commit "[p2p] overhaul TxOrphanage with smarter limits"
Since these types are used in the return types of (the implementation of) `TxOrphanage` interface functions, maybe they belong in `TxOrphanage` directly, so that e.g. both `TxOrphanage::UsageFromPeer` and `TxOrphanageImpl::UsageFromPeer` can use `Usage` as return type?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138025794)
In commit "[p2p] overhaul TxOrphanage with smarter limits"
Since these types are used in the return types of (the implementation of) `TxOrphanage` interface functions, maybe they belong in `TxOrphanage` directly, so that e.g. both `TxOrphanage::UsageFromPeer` and `TxOrphanageImpl::UsageFromPeer` can use `Usage` as return type?
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138105074)
In commit "[p2p] overhaul TxOrphanage with smarter limits"
Nit: maybe point out that it isn't strictly necessary to use 1 as a lower limit, because when the last peer goes below ratio 1, necessarily `NeedsTrim()` will be false, and we'd stop anyway. But it's more convenient to have some fallback number to use anyway.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138105074)
In commit "[p2p] overhaul TxOrphanage with smarter limits"
Nit: maybe point out that it isn't strictly necessary to use 1 as a lower limit, because when the last peer goes below ratio 1, necessarily `NeedsTrim()` will be false, and we'd stop anyway. But it's more convenient to have some fallback number to use anyway.
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138070999)
In commit "[p2p] overhaul TxOrphanage with smarter limits"
I'm confused by this comment. Neither `MaxPeerAnnouncements()` or `ReservedPeerUsage()` are affected by the size of `m_peer_orphanage_info`, so even if that map's size were to change (which can happen in this function...), nothing would change? In fact, they're both just returning `const` class member variables.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138070999)
In commit "[p2p] overhaul TxOrphanage with smarter limits"
I'm confused by this comment. Neither `MaxPeerAnnouncements()` or `ReservedPeerUsage()` are affected by the size of `m_peer_orphanage_info`, so even if that map's size were to change (which can happen in this function...), nothing would change? In fact, they're both just returning `const` class member variables.