💬 ryanofsky commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1483588570)
In commit "wallet: db, introduce 'RunWithinTxn()' helper function" (a8a24cb20974cb8eaec8cb438f6b22489719d605)

Maybe use string_view instead of string, since in most cases this is just passed a string literal, so no need to create a full string object

💬 jonatack commented on pull request "RFC: build: remove confusing and inconsistent disable-asm option":
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1934956278)
My usual fuzz configure for ARM64 macOS is the one in `doc/fuzzing.md`.

Just tried building with and without the `--disable-asm` option and then running `FUZZ=random ./src/test/fuzz/fuzz`.

The one difference I notice without `--disable-asm` is this additional entry near the beginning of the fuzz run (after which the fuzzer continues to run):

```
crc32c/src/crc32c_arm64.cc:101:26: runtime error: load of misaligned address 0x62d000000406 for type 'uint64_t' (aka 'unsigned long long'), wh
...

💬 fjahr commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1483608953)
There is an extra space after the extra_args option.

Since the `-persistmempool=0` is only relevant to this one restart and not the rest of the tests I wouldn't change the setup and instead do it like this, but how it's done now is also ok.

Another small improvement might be to switch from node 2 to node 0. None of the args from node 2 are needed for this test, so using node 0, that has no args, may cause less confusion.

```suggestion
# Restart to purge the transaction from th
...

💬 brunoerg commented on pull request "i2p: log connection was refused due to arbitrary port":
(https://github.com/bitcoin/bitcoin/pull/29393#discussion_r1483610162)
Good suggestion.

💬 fjahr commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1483610267)
Actually, when you switch to use node 0 you can remove this line entirely because node 0 is not used to load snapshots in the rest of the test.

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483614824)
Hmmm I find early returns in the middle of a function a bit brittle. Would be easier to write a bug if e.g. we added another v3 rule at the bottom of the function.

💬 fjahr commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1483614887)
@hernanmarino you marked the other thread as resolved while I was writing, this change works and I think that's what @maflcko had in mind: https://github.com/fjahr/bitcoin/commit/a6883b8919d7348bab56f6ebee0818912146ae52

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483617248)
fair enough, I just keep forgetting I can assume it has a parent because this block is pretty large. maybe a skill issue on my part

💬 dergoegge commented on pull request "RFC: build: remove confusing and inconsistent disable-asm option":
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1934972190)
@jonatack see https://github.com/bitcoin/bitcoin/issues/29178 (we prefer having this broken over reverting the change that introduced it for some reason)

I'm not sure if the `random` harness would trigger the ASan error, the docs mention sha256 which `random` does not use afaict. Could you try `txorphan` or a different harness that does some hashing?

Did you set `UBSAN_OPTIONS` to include `halt_on_error=1`? that is likely why it keeps running if you didn't

💬 brunoerg commented on pull request "i2p: log connection was refused due to arbitrary port":
(https://github.com/bitcoin/bitcoin/pull/29393#issuecomment-1934972685)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/29393#discussion_r1483580775

💬 brunoerg commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1483623515)
Sounds good, I'll address it.

💬 brunoerg commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1483624485)
I'll fix it

💬 jonatack commented on pull request "i2p: log connection was refused due to arbitrary port":
(https://github.com/bitcoin/bitcoin/pull/29393#issuecomment-1934983441)
ACK 5b358cdd1a5f5d2fe87a9e41c638996eab2e2796

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

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

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483628734)
added a comment

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

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

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483628403)
Gonna mark this as resolved 👍

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483631193)
I agree that in the case above D shouldn't need to conflict with all children, so will leave as is.