💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477998490)
Any reason to change the sequence id? I think this isn't changed in `loadtxoutset` either?

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477981888)
Any reason to touch this LOC?

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477998941)
Same (sequence id)

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477996772)
```suggestion
index->nChainTx = 0;
```

isn't this always true? Maybe you meant to type `-` instead of `<`?

In any case, the test seems to be passing either way, so this is "dead" code, similar to a code comment.

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478018977)
Can this be further limited to only mark the snap_block if it has ASSUMED_VALID set? Otherwise, nodes which never used loadtxoutset will assume that some block is a "snapshot block", even though they never used that feature.

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478006900)
Is removing the validity check intended?

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478022794)
Any reason to remove this check?

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1926747949)
> so i took time to test a NTA pinning scenario: [ariard/bitcoin@84e12b8](https://github.com/ariard/bitcoin/commit/84e12b8) which is known since https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2020-June/018011.html

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
...

💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1926771908)
Thanks Luke, I agree that repeated string concatenation was not a particularly neat for creating a single string where we could simply modify in place. I've updated this further from your version to use a lambda in bf9b11d8f6c0af9b2c76674481e5905a00240cf5.

Re Windows. Yes, fs::permissions is "supported", but IIUC without devolving into access control lists, it all boils down to whether the write bit is enabled. I don't see the sense in supporting either of these things here personally. The de
...

💬 delta1 commented on pull request "refactor: Allow CScript construction from any std::input_iterator":
(https://github.com/bitcoin/bitcoin/pull/29369#issuecomment-1926795738)
ACK [fa9ef95](https://github.com/bitcoin/bitcoin/pull/29369/commits/fa9ef95b4c1114e2d8f7c3307fdf01446efee6fd)

💬 maflcko commented on issue "getJsonToken assumes underlying string is null-terminated but requires end pointer":
(https://github.com/bitcoin/bitcoin/issues/28260#issuecomment-1926850992)
I guess this can be fixed by using std::string_view::starts_with

💬 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.