🤔 sr-gi reviewed a pull request: "net_processing: make any misbehavior trigger immediate discouragement"
(https://github.com/bitcoin/bitcoin/pull/29575#pullrequestreview-2001909800)
A couple of doc nits and a question below

💬 sr-gi commented on pull request "net_processing: make any misbehavior trigger immediate discouragement":
(https://github.com/bitcoin/bitcoin/pull/29575#discussion_r1566280997)
in: 78515e185755f5eba951917b8e2314ec69446d7d

nit: isn't this 99? (`for i in range(1, NUM_HEADERS)`)

💬 sr-gi commented on pull request "net_processing: make any misbehavior trigger immediate discouragement":
(https://github.com/bitcoin/bitcoin/pull/29575#discussion_r1566343004)
in: ce238dd79bd3a1190a2229fac4502fa5742fc6e3

I think the comment 6 lines below is wrong now:

```
// It should be impossible for the getheaders request to fail,
// because we should have cleared the last getheaders timestamp
// when processing the headers that triggered this call.
[...]
```

IIUC, `result.request_more` cannot be true if `result.success` is false (based on my understanding of [HeadersSyncState::ProcessNextHeaders](https://github.com/bitcoin/bitcoin/blob/07720b1cdd773
...

💬 sr-gi commented on pull request "net_processing: make any misbehavior trigger immediate discouragement":
(https://github.com/bitcoin/bitcoin/pull/29575#discussion_r1566293700)
in: ce238dd79bd3a1190a2229fac4502fa5742fc6e3

nit: I found this wording a bit hard to follow, especially the "to make marked responded to" bit.

IIUC, this is sending an empty block as a response to the outstanding getheaders request from the previous loop interaction, otherwise sending `blocks[i]` would be treated as the reply instead. Is that it? If so, I'd suggest something along the lines of:

```suggestion
# Send an empty header as a failed response to the received gethea
...

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2057691808)
> parsing the headers of overflow pages (which have btree level 0)

Interesting, did not know that they would use an invalid btree level. Fixed as suggested.

***

I've also commented out the enums that we do not use. I decided to leave them there for completeness, but they won't be used.

***

For help with reviewing, I suggest looking at the BDB source code. I've uploaded a copy of the version that we use at https://github.com/achow101/bdb-sources/tree/main/4.8.30.NC. The files that
...

💬 laanwj commented on issue "utils: wallet_dump can create a `database` directory, cross-pollinating records":
(https://github.com/bitcoin/bitcoin/issues/29883#issuecomment-2057708243)
Oh, i think part of the trigger here is that some of the wallets are backups of the same database at different times, so the logs can get re-used because the unique id matches.

💬 laanwj commented on pull request "netbase: clean up Proxy logging":
(https://github.com/bitcoin/bitcoin/pull/29882#discussion_r1566368437)
It's probably a good idea to keep the `!IsValid` check here, just to be sure. Fine with removing the log.

💬 maflcko commented on pull request "build: Fix false positive `CHECK_ATOMIC` test":
(https://github.com/bitcoin/bitcoin/pull/29859#issuecomment-2057777762)
Ok, I could reproduce by setting this CI to lunar: `FILE_ENV="./ci/test/00_setup_env_i686_multiprocess.sh" ./ci/test_run_all.sh`. However, on jammy, mantic, and noble it passes.

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2057799622)
There was a comment at CoreDev that this should check that the file is compacted, so I've added a commit that does that. To check that this is correct, see the implementation of [`lsn_reset`](https://github.com/achow101/bdb-sources/blob/main/4.8.30.NC/db/db_setlsn.c#L114), which uses [`LSN_NOT_LOGGED`](https://github.com/achow101/bdb-sources/blob/main/4.8.30.NC/dbinc/db_int.in#L803). The definition of [`DB_LSN`](https://github.com/achow101/bdb-sources/blob/main/4.8.30.NC/dbinc/db.in#L444) is als
...

⚠️ hebasto opened an issue: "`test/streams_tests.cpp` fails to compile on SunOS / illumos"
(https://github.com/bitcoin/bitcoin/issues/29884)
While testing bitcoin/bitcoin#29484, which seems not related to this bug, I've notice the following failure:
```
$ ./autogen.sh
$ ./configure
$ gmake -C src test/test_bitcoin
...
CXX test/test_bitcoin-streams_tests.o
In file included from test/streams_tests.cpp:5:
./streams.h: In instantiation of 'SpanReader& SpanReader::operator>>(T&&) [with T = signed char&]':
test/streams_tests.cpp:148:15: required from here
./streams.h:114:22: error: no matching function for call to 'Unser
...

💬 mzumsande commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1566233731)
Should update `ProcessInvalidTx` doc above too (it lists all member variables that the function updates).

🤔 mzumsande reviewed a pull request: "p2p: opportunistically accept 1-parent-1-child packages"
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2001371851)
Started reviewing - haven't looked at the tests yet.

💬 mzumsande commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1566300445)
have you considered returning early here if `cpfp_candidates` is empty? It should work regardless, but seems conceptually simpler than checking all the steps below can handle that case (and may be slightly faster too).

💬 mzumsande commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1566025027)
I think that they are sorted by internal order, not reversed-byte order (because they are sorted by `Wtxid` / `uint256`, not by `uint256.GetHex()`). Is that on purpose? In any case, maybe it would be useful to specify in the doc which order is used to avoid possbile confusion.

Also, this could the tests added in this commit only test for relative order between multiple transactions but not if they are actually sorted in lexicographical order, so that could also be done.

💬 mzumsande commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1565949969)
I asked myself the same question, found this discussion too late. I also think it would be good to mention this in the commit message (so it becomes clear that it has nothing to do with whether `m_replaced_transactions` has entries or is empty).

💬 mzumsande commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1566329125)
Could we assume more strictly here that the package size is 2 (for now)?

💬 mzumsande commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1566405746)
Why do we only try once in the case there are multiple children in the orphanage, instead of trying multiple times until one package succeeds? To avoid some kind of spamming attacks that could exhaust our computing power?

💬 mzumsande commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1566378431)
Why put the package hash into `m_recent_rejects_reconsiderable` instead of `m_recent_rejects`?
We never reconsider a failed package after all as far as I understand it.

💬 achow101 commented on pull request "RPC/Wallet: Add "use_txids" to output of getaddressinfo":
(https://github.com/bitcoin/bitcoin/pull/22693#discussion_r1566440347)
This comment was marked as resolved but nothing was changed.

💬 achow101 commented on pull request "RPC/Wallet: Add "use_txids" to output of getaddressinfo":
(https://github.com/bitcoin/bitcoin/pull/22693#issuecomment-2057817221)
Approach NACK

The [review](https://github.com/bitcoin/bitcoin/pull/22693#pullrequestreview-1246558912) I left last year outlines my concerns with this approach, and they have not been addressed in any way.