💬 ryanofsky commented on pull request "ci: remove unnecessary git diff after applying patch":
(https://github.com/bitcoin/bitcoin/pull/29461#issuecomment-1957079890)
> It is there to clarify the CI is using different code than the one in the source dir.

Oh that makes sense. If you think it would be an improvement, I could change the patch command to `echo '...' | tee >(patch -p1)` so the patch is copied to stdout

💬 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? :)