💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2058305830)
> There was a comment at CoreDev that this should check that the file is compacted, so I've added a commit that does that.

That's good to have. It does look like the newly added check trips up in the CI in some places:
```
unknown location(0): fatal error: in "db_tests/db_cursor_prefix_range_test": class std::runtime_error: LSNs are not reset, this database is not completely flushed. Please reopen then close the database with a version that has BDB support
```

💬 laanwj commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#issuecomment-2058329366)
This is something that we should have done a long time ago imo. I wouldn't be surprised if I have an ancient PR doing just this.
Concept ACK!

💬 paplorinc commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2058381680)
Hey @laanwj, thanks for your first review.
Please see the commits separately, don't worry, they're not all over the place.

💬 maflcko commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#discussion_r1566832562)
Why this change? This style change makes the code harder to read.

Unless there is a reason to change the code and review it, it shouldn't be changed.

💬 maflcko commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2058390006)
@paplorinc If you are not being receptive to reviewers' feedback, it is unlikely that anyone will review the pull request and without review it will not be merged.

💬 paplorinc commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#discussion_r1566838275)
I've changed it to unify it with the previous lambda. Why would I leave the type there when I've used auto 2 lines above?

💬 paplorinc commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2058398857)
@maflcko, I've responded to every single comment within minutes. I just don't appreciate the patronizing style I keep seeing in this repo.

🤔 fjahr reviewed a pull request: "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/28670#pullrequestreview-2002828900)
re-ACK 521de52c751a4539333bb2f69a07c0f1b4f496de

💬 fjahr commented on pull request "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28670#discussion_r1566860922)
nit: I think this info log could be improved if you retouch. Testing the file size is not really the goal, it's about hitting the metadata parsing error that was added, right? So I would suggest to call it something like "Inablity to parse metadata fails early" or so.

💬 maflcko commented on issue "`test/streams_tests.cpp` fails to compile on SunOS / illumos":
(https://github.com/bitcoin/bitcoin/issues/29884#issuecomment-2058434577)
Using `char` is not allowed in serialization code, because the sign of `char` is unknown/unclear. However, on some systems, `int8_t==char`, which means that `signed char` has no serialization helper.

This could be fixed in the serialization code by replacing `int8_t` by a concept of `!CharNotInt8 || std::same_as<T, signed char>`. However, my preference would be to completely avoid `signed char` and just use `int8_t` in the serialization code. That is, fix the test to use `int8_t`.

💬 laanwj commented on pull request "build: special instruction check script":
(https://github.com/bitcoin/bitcoin/pull/26693#issuecomment-2058437328)
BTW: along with some capstone magic like #29874, and a bare-bones DWARF (or linker map) parser to get function ranges, it would be possible to make a check like this that runs in the final binary, checking that special instructions only appear in allow-listed functions.

💬 maflcko commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#discussion_r1566874051)
My comment applies to all changes in this file. I am just trying to explain why such style changes do not make sense.

💬 laanwj commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2058447165)
Take it how you wish, i was just trying to be helpful, not patronizing.

💬 maflcko commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2058449206)
Please understand that this software project requires review of all code changes. It is not a typical software project that auto-merges any change as soon as the CI is green.

When writing a patch as a pull request, please consider how reviewers are supposed to review it and how easy it will be for them to follow the changes and their motivation.

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1566889900)
Yes exactly, this is to bound computation (imagine if somebody sent us 100 fake orphans descended from 1 transaction and we processed them all here). My idea was to do something similar to regular orphan processing, where we have a work queue and limit to 1 item per `ProcessMessages`.

There is no work queue here, though, and we drop the parent as soon as we try 1 (pass or fail). At coredev, we discussed adding a work queue for 1p1c as well. However, since it involves finding a way to store th
...

💬 fanquake commented on issue "ci: feature_proxy failing in MSVC job":
(https://github.com/bitcoin/bitcoin/issues/29090#issuecomment-2058480638)
https://github.com/bitcoin/bitcoin/actions/runs/8690581373/job/23830847391:
```bash
test 2024-04-15T15:06:28.426000Z TestFramework.node3 (DEBUG): Stopping node
test 2024-04-15T15:06:28.426000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "D:\a\bitcoin\bitcoin\test\functional\test_framework\test_framework.py", line 565, in start_nodes
node.
...

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1566907728)
Yes I think we could use either `m_recent_rejects_reconsiderable` and `m_recent_rejects` right now to get the same behavior.

I suppose one mild benefit of using `m_recent_rejects_reconsiderable` is that our `m_recent_rejects` bloom filter churns less frequently.

The other benefit is extensibility in the future. In more general ancestor package relay, we could reject a parent+child for being too low feerate, but later accept it as parent+child+grandchild (where the grandchild is very high f
...

💬 maflcko commented on issue "ci: feature_proxy failing in MSVC job":
(https://github.com/bitcoin/bitcoin/issues/29090#issuecomment-2058490446)
Maybe related to the races seen in https://github.com/bitcoin/bitcoin/issues/29871#issuecomment-2057480445 ?

📝 fanquake opened a pull request: "doc: archive 27.0 release notes"
(https://github.com/bitcoin/bitcoin/pull/29886)

💬 laanwj commented on pull request "guix: replace GCC unaligned VMOV patch with binutils patch":
(https://github.com/bitcoin/bitcoin/pull/29846#issuecomment-2058516921)
- I get the same output (for win32) as @hebasto
- I've checked the resulting binaries for instructions that enforce aligned memory accesses, and didn't find any dangerous ones

Code review ACK a0dc2ebcda9e33aa5320221cd4ea371f84d221fd