💬 maflcko commented on pull request "ci: remove unnecessary git diff after applying patch":
(https://github.com/bitcoin/bitcoin/pull/29461#issuecomment-1957099158)
Sure, anything is fine here.

💬 kevkevinpal commented on pull request "test: check_mempool_result negative feerate":
(https://github.com/bitcoin/bitcoin/pull/29459#discussion_r1497855816)
oops fixed in [bf264e0](https://github.com/bitcoin/bitcoin/pull/29459/commits/bf264e05981e3809715f34f548138d53991db6f2)

💬 maflcko commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#issuecomment-1957135673)
gh-sync

💬 maflcko commented on pull request "test: check_mempool_result negative feerate":
(https://github.com/bitcoin/bitcoin/pull/29459#issuecomment-1957144550)
lgtm ACK bf264e05981e3809715f34f548138d53991db6f2

💬 fjahr commented on pull request "RPC: access RPC arguments by name":
(https://github.com/bitcoin/bitcoin/pull/29277#issuecomment-1957172564)
> I doubt this will have any impact. It could even perform worse, and require more code?

Same amount of code and I don't see how this would perform worse: https://github.com/fjahr/bitcoin/commit/cbda2cf2c012030544ffc03620cbfcb91b1984be I also find this easier to reason about.

> Also, maps aren't constexpr, so if this is moved toward constexpr/consteval, it will have to be re-written again.

I guess I don't have enough insight into future development plans of the RPC to judge that if is.
...

💬 ryanofsky commented on pull request "ci: avoid running git diff after patching":
(https://github.com/bitcoin/bitcoin/pull/29461#issuecomment-1957175791)
Updated c835176774a4803132343d5e096cdc7fe902e8eb -> 84388c942cb035fed546eda360ae6b40c6cfac09 ([`pr/nodiff.1`](https://github.com/ryanofsky/bitcoin/commits/pr/nodiff.1) -> [`pr/nodiff.2`](https://github.com/ryanofsky/bitcoin/commits/pr/nodiff.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/nodiff.1..pr/nodiff.2)) restoring diff output with tee

💬 fjahr commented on pull request "contrib: add test for bucketing with asmap":
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1957241295)
tACK facfc2676b95949fa814a7686334d85d99e52519

CI failure seems unrelated...

💬 maflcko commented on pull request "RPC: access RPC arguments by name":
(https://github.com/bitcoin/bitcoin/pull/29277#issuecomment-1957248978)
> Same amount of code and I don't see how this would perform worse: [fjahr@cbda2cf](https://github.com/fjahr/bitcoin/commit/cbda2cf2c012030544ffc03620cbfcb91b1984be) I also find this easier to reason about.

You removed the `CHECK_NONFATAL` to mark the bug as an internal bug.

> which comes out to O(n^2)

The `O` notation only makes sense for large `n`. Is there more than one RPC with more than n=3? Again, if you measure it, I wouldn't be surprised if the runtime is slower for a map. Thoug
...

👍 ryanofsky approved a pull request: "doc: Assert that assumed-valid blocks are not fully valid in CheckBlockIndex()"
(https://github.com/bitcoin/bitcoin/pull/29355#pullrequestreview-1893776410)
Code review ACK fa027e08f7be63c201f42d0e06160d2273b4a6dd. This change makes sense and is an improvement over the status quo. I think it could be considered it a "doc" change since it is just adding an assert, which is a form of documentation, and not changing actual code. Not sure if that is how "doc" has been used other places, though.

This PR is a little redundant with #29370, which drops the BLOCK_ASSUMED_VALID flag entirely, so there is no way it could conflict with BLOCK_VALID_SCRIPTS. B
...

💬 theuni commented on pull request "RFC: build: remove confusing and inconsistent disable-asm option":
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1957262580)
> > > In the libsecp repo, the `--with-asm=no` option is still required to avoid MSan warnings.
> >
> >
> > Can you point to where this is an issue? It looks as though secp has proper `__msan_*` annotations in place these days.
>
> Sure. For example, [bitcoin-core/secp256k1#1169 (comment)](https://github.com/bitcoin-core/secp256k1/pull/1169#issuecomment-1370003449). It holds for the current libsecp master branch @ [bitcoin-core/secp256k1@0653a25](https://github.com/bitcoin-core/secp256k1
...

✅ maflcko closed a pull request: "doc: Assert that assumed-valid blocks are not fully valid in CheckBlockIndex()"
(https://github.com/bitcoin/bitcoin/pull/29355)

💬 maflcko commented on pull request "doc: Assert that assumed-valid blocks are not fully valid in CheckBlockIndex()":
(https://github.com/bitcoin/bitcoin/pull/29355#issuecomment-1957266392)
> This PR is a little redundant with https://github.com/bitcoin/bitcoin/pull/29370, which drops the BLOCK_ASSUMED_VALID flag entirely, so there is no way it could conflict with BLOCK_VALID_SCRIPTS. But it makes sense to check for conflicting flags as long as both flags exist.

Yeah, makes sense. I forgot to close this pull when the other was opened.

⚠️ achow101 pinned an issue: "Gathering Priorities of 28.0"
(https://github.com/bitcoin/bitcoin/issues/29439)
Please nominate projects that you are the champion of that could become priorities for the next ~6 months (until the 28.0 release). We (frequent contributors) will vote on which projects will be priorities next week in a separate issue.

There were concerns in the voting of previous priority projects that some projects nominated did not have champions who were willing to have their project be a priority project. To ensure that all nominated projects have champions who want their project to be
...

💬 achow101 commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1497971188)
Fixed

💬 achow101 commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-1957402165)
Changed all of the remaining tx.nVersion that I could find.

💬 glozow commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1497979346)
I've repurposed this test to check that sibling eviction is not allowed through multi-testmempoolaccept or submitpackage

💬 glozow commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1497979453)
Added checks that all the others have `nullptr`. Also added unit tests for whether sibling is returned when {1p1c, 1p2c, 3gen} is in mempool.

💬 glozow commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1497979615)
added

💬 theuni commented on pull request "RFC: build: remove confusing and inconsistent disable-asm option":
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1957432296)
Any other sanitizer false-positives while we're at it? :)

🤔 mzumsande reviewed a pull request: "Choose earliest-activatable as tie breaker between equal-work chains"
(https://github.com/bitcoin/bitcoin/pull/29284#pullrequestreview-1894004543)
> The attacker mines blocks B and C in secret, broadcasts the header of B, and the full block data of C. Other miners cannot build on top of this (except empty blocks). As soon as the rest of the network catches up (by mining B' and C'), the attacker broadcasts B. With the current logic, the attacker's A-B-C chain will become the active chain.

I don't think that would happen. Since #6010 `nSequenceId` is only set if the block and all of its predecessors have transactions, see [here](https://g
...