💬 MarcoFalke commented on pull request "Remove almost all blockstorage globals":
(https://github.com/bitcoin/bitcoin/pull/25781#discussion_r1135983875)
> there is little benefit ...

Thanks, done.

💬 pinheadmz commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1135993681)
added in force-push to 5e579c6435145d84e43fb79224f2cb09b40ea047 and rebased on master

💬 desirepl commented on issue "Bitcoin ignores datadir and blocksdir parameter in .conf":
(https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1468646427)
@hebasto git clone --depth 1 > build > run & tested
all the same, it ignores .conf's datadir

@pinheadmz add custom datadir too
and look where bitcoin will write it's data - to the non-default path you entered before or to the conf's path

💬 achow101 commented on pull request "guix: use osslsigncode 2.5":
(https://github.com/bitcoin/bitcoin/pull/27179#issuecomment-1468650553)
ACK 285edfadcacde4921c0afa2092c613daf21a55aa

```
c1471783bd078d094d886dc010ba6798c6d6abbd3b5329d7ce0ff3df05a3bcd9 guix-build-285edfadcacd/output/dist-archive/bitcoin-285edfadcacd.tar.gz
151e208ad965f1c89dda0ea01cff659930cdce060e1fc32e7db474fe5814a4a1 guix-build-285edfadcacd/output/x86_64-w64-mingw32/SHA256SUMS.part
d83fb0326d5c195d32a3243d494c34b6d0810c0aa36e174ff0d1eb1664f40413 guix-build-285edfadcacd/output/x86_64-w64-mingw32/bitcoin-285edfadcacd-win64-debug.zip
6f557b84042874ffbb675
...

👋 fanquake's pull request is ready for review: "guix: use osslsigncode 2.5"
(https://github.com/bitcoin/bitcoin/pull/27179)

💬 TheCharlatan commented on pull request "Remove almost all blockstorage globals":
(https://github.com/bitcoin/bitcoin/pull/25781#discussion_r1136093845)
I'm taking a look at this with some fresh knowledge now. If I understand this correctly, this always overrides what is passed in the `opts` with `0` if the `prune` arg is not set. The way I understand the current patterns elsewhere in the code for reading the arguments, the passed in opts are preserved if no argument is found, for example as done in [chainstatemanager_args](https://github.com/bitcoin/bitcoin/blob/master/src/node/chainstatemanager_args.cpp) / [chainstatemanager_opts](https://gith
...

💬 mzumsande commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136136446)
The init value should be `CAmount{0}`, not `0` which would be converted to an `int`, see [std::accumulate](https://en.cppreference.com/w/cpp/algorithm/accumulate) under "Common mistakes". This caused the unexpected fuzzer behavior I saw, because the `int` could overflow.
Also, for `ancestor_package_size`, the init value should be `int64_t{0}`;

💬 mzumsande commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136146262)
I commented below for this. After fixing this, the fuzzer doesn't crash anymore (at least not immediately, haven't run it for long).
I wonder if it might make sense to adjust the fuzz test permanently similar to my branch above and assert `sum(CalculateBumpFees) >= CalculateTotalBumpFees()`, if that is really the expectation - in general, I like it if fuzz tests contain asserts test actual non-trivial high-level expectations we have for the code, and don't just run over the code "blindly".

💬 pinheadmz commented on issue "Bitcoin ignores datadir and blocksdir parameter in .conf":
(https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1468791319)
@desirepl ok got it, I can reproduce. I think this is only an issue with the GUI. I also noticed one or two other little oddities around config file and will open a PR soon.

⚠️ fanquake opened an issue: "feature_config_args.py failure under `--valgrind`"
(https://github.com/bitcoin/bitcoin/issues/27259)
This doesn't fail inside the Valgrind CI system, but seems to consistently fail in a (non-timeout) fashion when running the functional tests under `--valgrind`. Done on Ubuntu 22.04, with Vagrind `valgrind-3.21.0.GIT`. master at 460e394625fab2942748aaeec9be31f460f91c58.
```bash
254/262 - feature_config_args.py failed, Duration: 54 s

stdout:
2023-03-14T20:40:35.820000Z TestFramework (INFO): PRNG seed is: 3139282899219230712
2023-03-14T20:40:35.821000Z TestFramework (INFO): Initializing tes
...

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136182559)
I think I need to touch five or six different places for this. Is that worth it?

💬 mzumsande commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1136210879)
This is just being moved here and I don't think this can overflow in practice, but the init value should be `int64_t{0}`, so maybe it would make sense to fix this somewhere within this PR. (searched the rest of the codebase after https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136136446).

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1468854086)
> What do you think?

As @stickies-v explained to me outside this PR: "... by moving the uri parsing to http_request_cb we're failing super early, and even when the query parameter would never be called by the endpoint... ", this makes more sense.

I'm working on this approach which I think it's better as well.

💬 desirepl commented on issue "Bitcoin ignores datadir and blocksdir parameter in .conf":
(https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1468925984)
@pinheadmz bitcoin-cli also ignores .conf's datadir, so not only GUI
maybe bitcoind too, didn't checked it, because spent too much time for -qt to understand why it's not working as must be

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136214058)
I’ve updated the comment to mention the 500 entry limit and the resulting empty vector.

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136311603)
Thanks, I adopted your suggestion.

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136215648)
Good catch. I’ve removed the line and amended the comment to clarify that the transaction is part of the descendants.

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136294354)
Thanks I took your suggestion with slightly more elaborate variable names.

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136217858)
Thanks, I’ve adopted your suggestion and moved `!mempool.exists` first.

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136319884)
Thanks, this improves the fuzzer. I’ve adopted your suggested change.