💬 mzumsande commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1132925968)
I don't understand what this part of the test is testing - calling `SaveBlockToDisk` for a block at a specified but incorrect position is something that should never be possible in an actual, non-corrupted node unless I'm missing something.
But if it somehow happened anyway, even if the existing block isn't overwritten, I'd imagine that it would still cause some havoc, like changing block file statistics [here](https://github.com/bitcoin/bitcoin/blob/c7f1d95f52883d7087b74f45f5e4ce1100d51149/sr
...

💬 TheCharlatan commented on pull request "ci: Cache more stuff in the ci images: msan, iwyu, pip, sdks":
(https://github.com/bitcoin/bitcoin/pull/27028#issuecomment-1464563233)
On master I can successfully run `MAKEJOBS="-j12" FILE_ENV="./ci/test/00_setup_env_native_tidy.sh" ./ci/test_run_all.sh`. When I do this now (with all prior images removed) I get:

```
python3: can't open file '/home/drgrid/bitcoin/ci/scratch/iwyu/include-what-you-use/iwyu_tool.py': [Errno 2] No such file or directory
python3: can't open file '/home/drgrid/bitcoin/ci/scratch/iwyu/include-what-you-use/fix_includes.py': [Errno 2] No such file or directory
```

💬 mzumsande commented on pull request "blockstorage: add an assert to avoid running oom with `-fastprune`":
(https://github.com/bitcoin/bitcoin/pull/27191#discussion_r1132967989)
Nice idea! I think that would work - I could do a full signet sync with it.

I'm still a bit undecided on general grounds, i.e. if it's okay to add more logic to accommodate a test-only mode that doesn't make sense for anything else but easier testing of pruning in the functional and unit tests. One could also argue that if no one but a developer would ever have a reason to use it, it'd be ok to just tell them more clearly not to create large blocks alongside using this option and abort otherw
...

💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1132968822)
oh interesting, I thought that the compiler optimizes away these sort of named-variables-used-shortly-after patterns, but maybe it's different for `std::string`? I took it to compiler explorer to see if I could observe the allocation- https://godbolt.org/z/GG78zv3KT. are the calls to `std::allocator<char>` what you are referring to?

⚠️ Lokoluis opened an issue: "Concept ACK"
(https://github.com/bitcoin/bitcoin/issues/27241)
Concept ACK

_Originally posted by @fanquake in https://github.com/bitcoin/bitcoin/issues/27233#issuecomment-1464352514_

✅ fanquake closed an issue: "Concept ACK"
(https://github.com/bitcoin/bitcoin/issues/27241)

:lock: fanquake locked an issue: "Concept ACK"
(https://github.com/bitcoin/bitcoin/issues/27241)

💬 fanquake commented on pull request "test: fix race condition in encrypted wallet rescan tests":
(https://github.com/bitcoin/bitcoin/pull/27199#issuecomment-1464875015)
Tested 308ea03d95786db674e8cce3e78a56b498cf119a rebased on master (c7f1d95f52883d7087b74f45f5e4ce1100d51149) and I'm no-longer seeing the issue in #27229: `wallet_importdescriptors.py --descriptors | ✓ Passed | 652 s`

🚀 fanquake merged a pull request: "doc: update broken str util reference links on developer-notes"
(https://github.com/bitcoin/bitcoin/pull/27220)

✅ fanquake closed an issue: "Bug: ArgsManager::ReadSettingsFile can return false even when it does load settings"
(https://github.com/bitcoin/bitcoin/issues/22638)

🚀 fanquake merged a pull request: "util: fix argsman dupe key error"
(https://github.com/bitcoin/bitcoin/pull/27236)

💬 fanquake commented on pull request "refactor: Consistently use context args over gArgs in node/interfaces":
(https://github.com/bitcoin/bitcoin/pull/27239#issuecomment-1464879171)
@TheCharlatan note that you've re-ACK'd the wrong commit hash.

🚀 fanquake merged a pull request: "refactor: Consistently use context args over gArgs in node/interfaces"
(https://github.com/bitcoin/bitcoin/pull/27239)

💬 MarcoFalke commented on pull request "blockstorage: add an assert to avoid running oom with `-fastprune`":
(https://github.com/bitcoin/bitcoin/pull/27191#discussion_r1133071807)
> @MarcoFalke : Do you have an opinion on this, or do you remember past discussions

Both no :)

💬 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_r1133072579)
1. What I'm asserting here is that if a position is passed in, the function called "save block to disk" does not save a block to disk. By using incorrect data I can easily prove that it is not actually written to disk.
2. The call to `AddBlock` you highlight here confuses and concerns me a bit - since we know a `known` block won't be written, why are we updating the metadata at all?

⚠️ theStack opened an issue: "Building `--with-experimental-kernel-lib` fails on OpenBSD 7.2"
(https://github.com/bitcoin/bitcoin/issues/27242)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Building with the `--with-experimental-kernel-lib` configuration option currently leads to linker failures on OpenBSD 7.2. The following compiler/linker/build-tool versions are used:
- clang 13.0.0
- lld 13.0.0
- ccache 4.6.3
- GNU make 4.3
- autoconf 2.71
- automake 1.16.5

### Expected behaviour

-

### Steps to reproduce

```
$ ./configure --enable-suppress-external-warnings --wi
...

⚠️ ponury1990 opened an issue: "wiadomośc"
(https://github.com/bitcoin-core/gui/issues/718)
https://github.com/bitcoin-core/gui/blob/9887fc789886b356efc6818783cabaab2b14481f/test/functional/p2p_invalid_messages.py#L90-L92

💬 theStack commented on pull request "test: add coverage for sigop limit policy (`-bytespersigop` setting)":
(https://github.com/bitcoin/bitcoin/pull/27171#issuecomment-1464905433)
> @theStack could open a followup addressing anything here?
>
> > (Doesn't need to be in this PR) it could be worth checking that this vsize calculation is used for anc/desc limits as well?
>
> > for reference, usually fAccurate is set for witness scripts sigop counts, but omitting the k and n in the CMS imitates fAccurate=false.

Yes, I'm planning to do a follow-up next week, extending the checks on ancestor/descendant limits is definitely a good idea! Regarding Marco's explanation w.r.
...

💬 davidgumberg commented on pull request "Improve address decoding errors":
(https://github.com/bitcoin/bitcoin/pull/26514#issuecomment-1464915896)
Maybe add a message for not-yet-understood witness versions and a test case?

```diff
if (version == 1 && data.size() == WITNESS_V1_TAPROOT_SIZE) {
static_assert(WITNESS_V1_TAPROOT_SIZE == WitnessV1Taproot::size());
WitnessV1Taproot tap;
std::copy(data.begin(), data.end(), tap.begin());
return tap;
}
+
+ if (version > 1 && version <= 16) {
+ error_str = "Invalid or unsupported Bech32 address witness version."
+ return CNoDestination();
+ }
+

...

💬 sipa commented on pull request "Improve address decoding errors":
(https://github.com/bitcoin/bitcoin/pull/26514#issuecomment-1464916288)
@davidgumberg That would be incorrect; sending to future witness versions is perfectly legal. It might make sense in the UI to give a warning, but we can't outlaw it (as it'd break future upgradability).