💬 pablomartin4btc commented on pull request "cli: add validation to cli side commands besides when it's used with -rpcwallet":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1135879724)
fixed

💬 pablomartin4btc commented on pull request "cli: add validation to cli side commands besides when it's used with -rpcwallet":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-1468452555)
> ACK [0fe610d](https://github.com/bitcoin/bitcoin/commit/0fe610deba336f0370d68c130dc9a223b7db964e) modulo the dangling closing parenthesis in the error message.
> Also, the first line of the commit message should be at most 80 chars.

I've corrected them both. Thanks!

💬 pinheadmz commented on issue "Bitcoin ignores datadir and blocksdir parameter in .conf":
(https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1468528728)
I couldn't reproduce this issue on macOS, here's what I tried:

- remove bitcoin plist file
- open bitcoin-qt with `-regtest`, setup wizard pops up, enter custom non-default path
- close bitcoin
- add `bitcoin.conf` file at custom path including the line `blocksdir=/tmp/blocks-test`
- restart bitcoin-qt

the result was expected, bitcoin used the specified blocks directory.

💬 Empact commented on pull request "refactor: Split logging utilities from system.h":
(https://github.com/bitcoin/bitcoin/pull/27238#issuecomment-1468541697)
Thanks @TheCharlatan for picking this up. 👍

📝 dergoegge opened a pull request: "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg"
(https://github.com/bitcoin/bitcoin/pull/27257)
We should define clear interfaces between CNode, CConnman and PeerManager. This PR makes a small step in that direction by ending the friendship of CNode, CConnman and ConnmanTestMsg. CNode's message processing queue is made private in the process and it's mutex is turned in to a non-recursive mutex.

💬 dergoegge commented on pull request "net, test: Virtualise CConnman and add initial PeerManager unit tests":
(https://github.com/bitcoin/bitcoin/pull/25515#discussion_r1135979252)
#27257 dedupes this code across the entire code base, reviewing that would help move this forward.

💬 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