:lock: fanquake locked an issue: "Concept ACK"
(https://github.com/bitcoin/bitcoin/issues/27241)
(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`
(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)
(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)
(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)
(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.
(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)
(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 :)
(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?
(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
...
(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
(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.
...
(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();
+ }
+
...
(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).
(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).
π¬ davidgumberg commented on pull request "Improve address decoding errors":
(https://github.com/bitcoin/bitcoin/pull/26514#issuecomment-1464918507)
@sipa Thanks! I misunderstood.
(https://github.com/bitcoin/bitcoin/pull/26514#issuecomment-1464918507)
@sipa Thanks! I misunderstood.
π¬ 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_r1133090332)
> 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?
Because the block index LevelDB database (saved in `blocks/index`) is being wiped at the beginning of a reindex and needs to be rebuilt from scratch as we look at each block. Part of this database is meta info about each blockfile ([see bitcoin wiki](https://en.bitcoin.it/wiki/Bitcoin_Core_0.11_(ch_2):_Data_Storage#Block_in
...
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1133090332)
> 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?
Because the block index LevelDB database (saved in `blocks/index`) is being wiped at the beginning of a reindex and needs to be rebuilt from scratch as we look at each block. Part of this database is meta info about each blockfile ([see bitcoin wiki](https://en.bitcoin.it/wiki/Bitcoin_Core_0.11_(ch_2):_Data_Storage#Block_in
...
π¬ 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_r1133091606)
ok make sense thanks. do you think my first point justifies the method of the test or is that just confusing then and I should remove it?
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1133091606)
ok make sense thanks. do you think my first point justifies the method of the test or is that just confusing then and I should remove it?
π¬ davidgumberg commented on pull request "refactor: Split logging utilities from system.h":
(https://github.com/bitcoin/bitcoin/pull/27238#issuecomment-1464927718)
ACK https://github.com/bitcoin/bitcoin/pull/27238/commits/485ea8d400241506a6abe485b7b71b0cacfdc853
(https://github.com/bitcoin/bitcoin/pull/27238#issuecomment-1464927718)
ACK https://github.com/bitcoin/bitcoin/pull/27238/commits/485ea8d400241506a6abe485b7b71b0cacfdc853
π¬ amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1133106788)
this test is actually covering the `tried_count == 0` logic. if it were to hit line 746, it would sometimes select the tried table and sometimes the new table, which means it should fail 50% of the time. this coverage is ensuring that the `tried_count` has the proper interactions with `network`, which gets assigned earlier in the function-
```
if (network.has_value()) {
auto it = m_network_counts.find(*network);
if (it == m_network_counts.end()) return {};
au
...
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1133106788)
this test is actually covering the `tried_count == 0` logic. if it were to hit line 746, it would sometimes select the tried table and sometimes the new table, which means it should fail 50% of the time. this coverage is ensuring that the `tried_count` has the proper interactions with `network`, which gets assigned earlier in the function-
```
if (network.has_value()) {
auto it = m_network_counts.find(*network);
if (it == m_network_counts.end()) return {};
au
...
π 1440000bytes approved a pull request: "Improve address decoding errors"
(https://github.com/bitcoin/bitcoin/pull/26514)
utACK https://github.com/bitcoin/bitcoin/pull/26514/commits/962a0930e699b74b3c4d019427df6e2b3af5c831
---
> NACK on talking about "different networks". While this could in theory be Lightning or sidechains, it's more likely to lead users toward scamcoins.
Ideally it should have been 'chain' instead of network as networks are ipv4, ipv6, tor, i2p etc. although we should not be concerned about scammers if they ever use this software but use correct technical terms in errors IMO.
The com
...
(https://github.com/bitcoin/bitcoin/pull/26514)
utACK https://github.com/bitcoin/bitcoin/pull/26514/commits/962a0930e699b74b3c4d019427df6e2b3af5c831
---
> NACK on talking about "different networks". While this could in theory be Lightning or sidechains, it's more likely to lead users toward scamcoins.
Ideally it should have been 'chain' instead of network as networks are ipv4, ipv6, tor, i2p etc. although we should not be concerned about scammers if they ever use this software but use correct technical terms in errors IMO.
The com
...