💬 maflcko commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1592642962)
> it might be good to clarify in a comment.

There is already a comment in line 19, which explains why this is done ("should be stored to std::unique_ptr members pointing to opaque types.")

While there currently is no unique_ptr, it is possible that one will be re-added in the future?

💬 willcl-ark commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1592662009)
Thanks for the feedback @maflcko, I agree with it all, mostly.

That said it was quite painful to integrate it into `lint/test_runner`, mainly because the `mlc` tool will only accept a _single_ file/dir argument as the target but will permit multiple `--exclude-path` options. Therefore the only way I could concieve of not linting random directories in the root src dir was to deduplicate the output of `git ls-tree -rtd --format='%(path)' HEAD ./` with all top-level directories found by `fs::rea
...

👍 alfonsoromanz approved a pull request: "test: added test coverage to loadtxoutset could not open file"
(https://github.com/bitcoin/bitcoin/pull/30053#pullrequestreview-2043495326)
ACK ee67bba76cca2355541f99bb731f58479981b29e. Code looks good to me. I also ran `test/functional/feature_assumeutxo.py` to make sure all tests passes, including this one.

<img width="943" alt="Screenshot 2024-05-07 at 12 07 04" src="https://github.com/bitcoin/bitcoin/assets/19962151/bbe9ced1-443d-43cc-9111-0e61e35fc7d6">
<img width="876" alt="Screenshot 2024-05-07 at 12 07 18" src="https://github.com/bitcoin/bitcoin/assets/19962151/36e2e6fb-39fa-4ec1-b25d-5247d7ded24d">

💬 ryanofsky commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1592676778)
re: https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1592642962

> There is already a comment in line 19, which explains why this is done ("should be stored to std::unique_ptr members pointing to opaque types.")
>
> While there currently is no unique_ptr, it is possible that one will be re-added in the future?

Oh interesting. I was wondering why NOLINT was needed here but not in NodeContext, but I guess it is because NodeContext does contain unique_ptr members, and this doesn't
...

💬 maflcko commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1592685484)
> Therefore the only way I could concieve of not linting random directories in the root src dir

If this is too difficult, it does not need to be implemented for now. The risk should be minimal, as `mlc` should only have read-only access? And if it were to fail for someone that put random other folders into the root, they can always skip the check?

💬 vasild commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1592701864)
I set `dstaddr` and `dstport` here in `connection_made()` because when accepting connections (from the python node point of view) they are set to `"0"` and `0` respectively (in `master`):

https://github.com/bitcoin/bitcoin/blob/ef09f535b7b540d997f8c2bfa67b3386711bc8b4/test/functional/test_framework/p2p.py#L203

https://github.com/bitcoin/bitcoin/blob/ef09f535b7b540d997f8c2bfa67b3386711bc8b4/test/functional/test_framework/p2p.py#L182-L186

When I add those asserts I get:
```
assert s
...

💬 vasild commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1592706360)
No, I just picked some value that looks reasonable, anything would work. For example `1` or `5` would still work but does not look reasonable because it will cause a lot of iteration loops.

💬 vasild commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1592709053)
I slightly prefer to keep the array unmodified in case the data in it is needed at some later point (for logging or something).

💬 willcl-ark commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1592709785)
Yes, I think this is probably the best approach too. I'll make those changes and push here soon (tm)

💬 luke-jr commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r1592709382)
Is there a reason to name it something different here? Especially while the option is still broken and doesn't actually limit datacarriers?

💬 maflcko commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-2098816463)
> [clusterfuzz-testcase-mocked_descriptor_parse-5224675118481408.bin.not.txt](https://github.com/bitcoin/bitcoin/files/13877884/clusterfuzz-testcase-mocked_descriptor_parse-5224675118481408.bin.not.txt)

Same for the non-mocked one:

[clusterfuzz-testcase-minimized-descriptor_parse-5888623437217792.bin.txt](https://github.com/bitcoin/bitcoin/files/15238132/clusterfuzz-testcase-minimized-descriptor_parse-5888623437217792.bin.txt)


![Screenshot from 2024-05-07 18-05-00](https://github.com/
...

⚠️ hebasto opened an issue: "build: Unaligned libsecp256k1 flags in debug builds"
(https://github.com/bitcoin/bitcoin/issues/30055)
When configured with `--enable-debug`, C++ code compiles with `-O0 -g3`. However, libsecp256k1 code compiles with `-g -O2`.

⚠️ jdom1824 opened an issue: "(EDITED) How was the average size of blk*.data chosen?"
(https://github.com/bitcoin/bitcoin/issues/30056)
(EDITED)

List of implementation changes:
- new database layout:
- 2 leveldb's (coins/ and blktree/ subdirs), replacing blkindex.dat
- separate directory (blocks/) with block data (in the usual format, but smaller files) and undo data
- database keys are of the form (char,key) instead of (string,key) for reasons of compactness
- there is no txid-to-diskpos index anymore, only blkid-to-diskpos and txid-to-unspent-outputs
- this makes getrawtransaction only work on unspent outputs (
...

💬 davidgumberg commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1592765102)
nit:
```suggestion
LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "pcp: Requesting port mapping for addr %s port %d from gateway %s\n", bind.ToStringAddr(), port, gateway.ToStringAddr());
```

💬 brunoerg commented on issue "`keypoolrefill` doesn't fill keypool to specified parameter":
(https://github.com/bitcoin/bitcoin/issues/29924#issuecomment-2098939485)
I think you have more than one `ScriptPubKeyManager`. When you call `keypoolrefill ` it will top up every spkm. Did you check `listdescriptors`?

💬 luke-jr commented on pull request "guix: build with glibc 2.31":
(https://github.com/bitcoin/bitcoin/pull/29987#issuecomment-2098941527)
Is there a benefit to this? Just dropping patches?

💬 luke-jr commented on pull request "guix: fix suggested fake date for openssl-1.1.1l":
(https://github.com/bitcoin/bitcoin/pull/29999#issuecomment-2098953741)
Is it possible to use a UTS namespace to spoof the clock just for the build?

💬 luke-jr commented on pull request "dbwrapper: Bump LevelDB max file size to 128 MiB to avoid system slowdown from high disk cache flush rate":
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2098989028)
If there's no drawbacks, why not go even larger?

💬 hebasto commented on issue "ci: Enable bpfcc-tools":
(https://github.com/bitcoin/bitcoin/issues/29804#issuecomment-2098990473)
> > Wait for 24.04 GHA image, Move the task over to that image
>
> Still waiting for it to appear here: https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners/about-github-hosted-runners#standard-github-hosted-runners-for-public-repositories

The wait is over: https://github.com/actions/runner-images/releases/tag/ubuntu24%2F20240430.1

💬 theStack commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1592872178)
This code snippet doesn't work for me on Sage 9.5 (see https://github.com/BlockstreamResearch/secp256k1-zkp/pull/292). Also, we only need to output the x-coordinate here:
```suggestion
G2 = EllipticCurve ([F (0), F (7)]).lift_x(F(int(hashlib.sha256(bytes.fromhex(G_DER)).hexdigest(),16)))
print('%x' % G2.xy()[0])
```
(That said, I think it's also completely fine to not include any sage code here and instead just keep the first paragraph.)