💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1557522115)
Just a note for other reviewers: This comment mentioning "our system" was slightly confusing to me: our serialization framework always serializes in LE, so if we see BTREE_MAGIC_OE it means the file is in big-endian. Which means we always need to swap it, independent of system's endianness.

💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1557537019)
i don't think importing `uint160` is necessary here--the actual value isnt used, we're just using it to consume some bytes

🚀 glozow merged a pull request: "refactor: Use typesafe Wtxid in compact block encodings"
(https://github.com/bitcoin/bitcoin/pull/29752)

💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2045074692)
Reference for reviewers:
- `db_page.h` from berkeleydb 4.8.30: https://gist.github.com/laanwj/2b998d6db8d9df384a30871035675aec

💬 fanquake commented on pull request "build: Change MAC_OSX macro to __APPLE__ in crypto":
(https://github.com/bitcoin/bitcoin/pull/29834#issuecomment-2045089673)
Guix Build (aarch64):
```bash
bdbdd644be801bd72472a94e878076406e63e8bc39c55f5a6e3bc28e23a72c81 guix-build-a71eadf66bed/output/aarch64-linux-gnu/SHA256SUMS.part
cf1eb27df84f25149a743be33f037eac960c1a943da2750b9030315593df4192 guix-build-a71eadf66bed/output/aarch64-linux-gnu/bitcoin-a71eadf66bed-aarch64-linux-gnu-debug.tar.gz
732f794906af544c3c0e41ff3cdc8fc0253fbef96d5572998286ad19ecbc5a9e guix-build-a71eadf66bed/output/aarch64-linux-gnu/bitcoin-a71eadf66bed-aarch64-linux-gnu.tar.gz
a50f89
...

💬 murchandamus commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1557578207)
calculation of two 32 bit integers, you need to use a cast

💬 glozow commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1557579240)
This should be in the extra args _above_ to avoid writing mempool.dat.

Currently the test is fine, but this is because the new datacarriersize causes all the transactions to be rejected.

💬 sipa commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1557581834)
You probably want `size_t pos = size_t{page_num} * page_size;`.

💬 davidgumberg commented on pull request "doc: fixup help output for -upnp and -natpmp":
(https://github.com/bitcoin/bitcoin/pull/28874#issuecomment-2045114437)
ACK https://github.com/bitcoin/bitcoin/commit/92f88a962908c49dde99c03a4608e63e4a6eec71

Configured `--with-natpmp` and verified the help messages.

```console
$ bitcoind --help

[...]
-natpmp
Use NAT-PMP to map the listening port (default: 0)
[...]
-upnp
Use UPnP to map the listening port (default: 0)
```

Verified that modifying `bool DEFAULT_UPNP` and `bool DEFAULT_NATPMP` changes the `--help` output.

💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1557583470)
aside: might be better to not use size_t at all for file offsets, as the type is 32 bit on usual 32 bit platforms

💬 glozow commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1557583670)
👍 I think this interface is better if the caller knows what the requirements are but can otherwise put whatever args they want

💬 instagibbs commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1557587565)
moved and added assertion of empty mempool after restart

💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1557592239)
do we want to sanity check for encryption to be disabled? (didn't know bdb supported db-level encryption in the first place)

👍 fanquake approved a pull request: "build: Change MAC_OSX macro to __APPLE__ in crypto"
(https://github.com/bitcoin/bitcoin/pull/29834#pullrequestreview-1988981666)
ACK a71eadf66bed8d3ea4282c8499f533a8eeed9900

💬 maflcko commented on pull request "build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29494#issuecomment-2045134493)
rebased

💬 maflcko commented on issue "valgrind: Conditional jump or move depends on uninitialised value(s)":
(https://github.com/bitcoin/bitcoin/issues/29635#issuecomment-2045231903)
https://bugs.kde.org/show_bug.cgi?id=485276

💬 real-or-random commented on issue "Fuzzer enhancement: Explicitly check output for uninitialized memory":
(https://github.com/bitcoin/bitcoin/issues/22064#issuecomment-2045244702)
> Alternative solution that also works with Valgrind: write the data to `/dev/null`:

There's also `VALGRIND_CHECK_MEM_IS_DEFINED`, see also https://github.com/bitcoin-core/secp256k1/blob/master/src/checkmem.h for an abstraction layer that works with both MSan and Valgrind.


---

What's suggested in this issue, i.e., reporting every read of an uninitialized value, may just be too much. The [Valgrind FAQ](https://valgrind.org/docs/manual/faq.html#faq.undeferrors) says this:
> As for eage
...

💬 theuni commented on pull request "[WIP] libevent @ master + use CMake":
(https://github.com/bitcoin/bitcoin/pull/29835#issuecomment-2045257973)
I'm confused about this error. sigaddset should come from `<signal.h>`, which is included from `"evsignal-internal.h"`.

💬 stratospher commented on pull request "test: Extends wait_for_getheaders so a specific block hash can be checked":
(https://github.com/bitcoin/bitcoin/pull/29736#discussion_r1557701025)
inserting an `assert "getheaders" not in p.last_message` fails here because it received a new header after those checks. and the block hash doesn't change here.

💬 stratospher commented on pull request "test: Extends wait_for_getheaders so a specific block hash can be checked":
(https://github.com/bitcoin/bitcoin/pull/29736#discussion_r1557701131)
oh good point.