💬 maflcko commented on pull request "serialization: c++20 endian/byteswap/clz modernization":
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1926859778)
I've created https://github.com/bitcoin/bitcoin/issues/26972 to track ideas on how to detect those silent build issues, or at least make them easier to spot.

💬 petertodd commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1926860376)
@glozow

> To summarize the "network topology aware" scenario, what happens with Alice and Bob's channel is:
>
> 1. Bob broadcasts his commitment transaction + cpfp from his anchor
>
> 2. This transaction propagates to everyone except Alice, somehow. [1]
>
> 3. Alice broadcasts her commitment transaction + cpfp from her anchor
>
> 4. Alice's transactions don't propagate because her commitment tx conflicts with Bob's commitment tx.
>
>
> This (1) requires the at
...

💬 maflcko commented on pull request "test: Assumeutxo with more than just coinbase transactions":
(https://github.com/bitcoin/bitcoin/pull/29354#issuecomment-1926882169)
rfm?

💬 maflcko commented on pull request "redeclare nChainTx to use uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29331#issuecomment-1926940411)
> The note about changing the type in 2024 was also removed. This passes all extended tests.

I don't think you need to detail what is changed and if the tests pass in the description. It would be better to focus on information that is useful to reviewers. For example, a measurement of memory before and after.

Also, when changing the type, you'll have to check all places where this is used, and update them, if needed. For example, casts should be removed and other types need to be increased
...

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1926945678)
(yes, see footnote [1])

💬 Sjors commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1926968881)
> This library has existed for nearly 10 years with very little known uptake or impact. It has become a maintenance burden. In several cases it dictates our code/library structure (for example necessitating LIBBITCOIN_CRYPTO_BASE), as well as build-system procedures (building multiple copies of object files especially for the lib).


What is the status of `LIBBITCOIN_CRYPTO_BASE`? Is it kept around or moved into libbitcoin_common?

💬 Sjors commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1926979724)
I see it's mentioned here: https://github.com/bitcoin/bitcoin/pull/29180#issuecomment-1878604328 - would be useful to also add a note to libraries.md

✅ maflcko closed a pull request: "refactor: Remove nChainTx from consensus"
(https://github.com/bitcoin/bitcoin/pull/29349)

💬 maflcko commented on pull request "refactor: Remove nChainTx from consensus":
(https://github.com/bitcoin/bitcoin/pull/29349#issuecomment-1926982235)
Closing for now, but happy to reopen if someone thinks this is useful.

💬 Sjors commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1927009876)
Concept ACK. Moving things out of util that the kernel will / should never need, makes sense to me. I don't have strong feelings on where exactly to move these things to.

💬 josibake commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1927058987)
> I think the newly added generic ser/unser methods to CKey are harmless.

Its adding a footgun to `CKey`. The class is making sure to keep the secret data secure in memory and to free it when the key is gone, so allowing the key to be created directly from (potentially insecure) data on disk and allowing the key to be written out unencrypted to disk without destroying the key seems like an anti-pattern. You could argue that the existing methods (e.g. `GetPrivKey`) are also footguns, but that
...

🤔 glozow reviewed a pull request: "test: Assumeutxo with more than just coinbase transactions"
(https://github.com/bitcoin/bitcoin/pull/29354#pullrequestreview-1862914988)
concept ACK fa5cd66f0a47d1b759c93d01524ee4558432c0cc

🚀 glozow merged a pull request: "test: Assumeutxo with more than just coinbase transactions"
(https://github.com/bitcoin/bitcoin/pull/29354)

🚀 glozow merged a pull request: "log: Don't use scientific notation in log messages"
(https://github.com/bitcoin/bitcoin/pull/29254)

🤔 furszy reviewed a pull request: "wallet: batch and simplify addressbook migration process"
(https://github.com/bitcoin/bitcoin/pull/26836#pullrequestreview-1862770273)
Updated per feedback, thanks for the review josie!

💬 furszy commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1478234209)
Because I made this commit before e83babe (#27217), which removed the `IsMine` calls, and I forgot to update the commit description after the rebase. Will clean it, thanks.

💬 furszy commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1478266195)
As we will never merge a PR solely removing the extra parentheses, we can either clean them now while working on this function or we keep them for another decade. I prefer to clean them now, but np in reverting the line if it conflicts with the review process.

💬 furszy commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1478340011)
> If the destination requires_transfer but isn't ours, go to the next wallet. If this happens for both wallets, copy will never be set and we will fail

If the destination requires transfer (a "receive" addr that is no longer part of the main wallet), the process checks to which wallet the entry belongs to. And, when the entry does not belong to any of the created wallets, it means that the key/script-to-descriptor migration process failed. This is because "receive" entries must always be trac
...

💬 furszy commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1478241812)
sure

💬 furszy commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1478350465)
sure