💬 ismaelsadeeq commented on pull request "Mempool: persist mempoolminfee accross restarts":
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1227810641)
Thank you. Yes, I agree that adding it at the end of `fee_estimates.dat` would indeed reduce redundancy. I would update that.
Regarding making it conditional to read the persisted `mempoolminfee`, could you please clarify the scenarios in which we might want to use the default `mempoolminfee` value? In this, it is already conditional to remember the `mempoolminfee` as stale `mempoolminfee` will not be read at all.
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1227810641)
Thank you. Yes, I agree that adding it at the end of `fee_estimates.dat` would indeed reduce redundancy. I would update that.
Regarding making it conditional to read the persisted `mempoolminfee`, could you please clarify the scenarios in which we might want to use the default `mempoolminfee` value? In this, it is already conditional to remember the `mempoolminfee` as stale `mempoolminfee` will not be read at all.
💬 MarcoFalke commented on pull request "fuzz: Fix mini_miner_selection running out of coin":
(https://github.com/bitcoin/bitcoin/pull/27806#issuecomment-1588895527)
lgtm ACK 76c5ea703e77d580b6962e60398f4988cbd9b58b
(https://github.com/bitcoin/bitcoin/pull/27806#issuecomment-1588895527)
lgtm ACK 76c5ea703e77d580b6962e60398f4988cbd9b58b
💬 furszy commented on pull request "net: introduce block tracker to retry to download blocks after failure":
(https://github.com/bitcoin/bitcoin/pull/27837#issuecomment-1588916036)
Oh, didn't know about the CI failure. Seems to be just an unused variable. But sure, moved to draft until https://github.com/bitcoin/bitcoin/pull/27836 get merged.
(https://github.com/bitcoin/bitcoin/pull/27837#issuecomment-1588916036)
Oh, didn't know about the CI failure. Seems to be just an unused variable. But sure, moved to draft until https://github.com/bitcoin/bitcoin/pull/27836 get merged.
💬 ismaelsadeeq commented on pull request "Mempool: persist mempoolminfee accross restarts":
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1227846644)
It appears that taking the approach of dumping to `fee_estimates.dat` and implementing a method to set `mempoolminfee` based on the persisted value is a better option.
Your perspective on persisting the mempool periodically. I understand the concern about remembering the fee estimates and `mempoolminfee` without the mempool itself. However, I would like to seek further insight into the potential consequences that could arise from this, as dumping the mempool periodically might be unappealing
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1227846644)
It appears that taking the approach of dumping to `fee_estimates.dat` and implementing a method to set `mempoolminfee` based on the persisted value is a better option.
Your perspective on persisting the mempool periodically. I understand the concern about remembering the fee estimates and `mempoolminfee` without the mempool itself. However, I would like to seek further insight into the potential consequences that could arise from this, as dumping the mempool periodically might be unappealing
💬 TheCharlatan commented on pull request "kernel: Add interrupt class":
(https://github.com/bitcoin/bitcoin/pull/27861#issuecomment-1589076014)
Rebased d60ab3b5099d3a348f3122b5d3207ac12f4a751c -> 3e60c02ef3759e16a762bd193e0b9291bf46b73c ([kernelInterrupt_0](https://github.com/TheCharlatan/bitcoin/commits/kernelInterrupt_0) -> [kernelInterrupt_1](https://github.com/TheCharlatan/bitcoin/commits/kernelInterrupt_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelInterrupt_0..kernelInterrupt_1)).
* Added `src/node/abort.*` to allow both the notification code and `index/base.cpp` to use a common abort definition as introduce
...
(https://github.com/bitcoin/bitcoin/pull/27861#issuecomment-1589076014)
Rebased d60ab3b5099d3a348f3122b5d3207ac12f4a751c -> 3e60c02ef3759e16a762bd193e0b9291bf46b73c ([kernelInterrupt_0](https://github.com/TheCharlatan/bitcoin/commits/kernelInterrupt_0) -> [kernelInterrupt_1](https://github.com/TheCharlatan/bitcoin/commits/kernelInterrupt_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelInterrupt_0..kernelInterrupt_1)).
* Added `src/node/abort.*` to allow both the notification code and `index/base.cpp` to use a common abort definition as introduce
...
💬 fanquake commented on pull request "wallet: Give deprecation warning when loading a legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/27869#issuecomment-1589076049)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27869#issuecomment-1589076049)
Concept ACK
📝 fanquake opened a pull request: "lint: suppress pip spam"
(https://github.com/bitcoin/bitcoin/pull/27871)
The logs are [currently full of](https://cirrus-ci.com/task/5380096597426176?logs=lint#L240):
```bash
WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager. It is recommended to use a virtual environment instead: https://pip.pypa.io/warnings/venv
WARNING: You are using pip version 22.0.4; however, version 23.1.2 is available.
You should consider upgrading via the '/tmp/python/bin/python3.8 -m pip install --upgrade p
...
(https://github.com/bitcoin/bitcoin/pull/27871)
The logs are [currently full of](https://cirrus-ci.com/task/5380096597426176?logs=lint#L240):
```bash
WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager. It is recommended to use a virtual environment instead: https://pip.pypa.io/warnings/venv
WARNING: You are using pip version 22.0.4; however, version 23.1.2 is available.
You should consider upgrading via the '/tmp/python/bin/python3.8 -m pip install --upgrade p
...
📝 fanquake opened a pull request: "build: suppress external warnings by default"
(https://github.com/bitcoin/bitcoin/pull/27872)
I think we are at the point where it make more sense to make this the default, than not. It's already used in the CI, and I assume most building locally are also utilising it.
(https://github.com/bitcoin/bitcoin/pull/27872)
I think we are at the point where it make more sense to make this the default, than not. It's already used in the CI, and I assume most building locally are also utilising it.
💬 fanquake commented on pull request "doc: clarify full Xcode download is not needed":
(https://github.com/bitcoin/bitcoin/pull/27835#issuecomment-1589083587)
> Some background, it should indeed not be necessary to install Xcode:
It hasn't been required for at least 7 years, that's why we removed the instructions to install in: https://github.com/bitcoin/bitcoin/commit/2692e1b10bd7a0be644ed8a69c54152bc741e855.
(https://github.com/bitcoin/bitcoin/pull/27835#issuecomment-1589083587)
> Some background, it should indeed not be necessary to install Xcode:
It hasn't been required for at least 7 years, that's why we removed the instructions to install in: https://github.com/bitcoin/bitcoin/commit/2692e1b10bd7a0be644ed8a69c54152bc741e855.
💬 fjahr commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1227953145)
@ajtowns Right, this is the example I thought of as well. But I don't understand why you think the check is unnecessary. From my understanding, this is the point where the subpackage A+B would fail then so I think it is necessary. It just depends on the fact that the function is called with each subpackage.
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1227953145)
@ajtowns Right, this is the example I thought of as well. But I don't understand why you think the check is unnecessary. From my understanding, this is the point where the subpackage A+B would fail then so I think it is necessary. It just depends on the fact that the function is called with each subpackage.
💬 0xB10C commented on pull request "build: Use newest `config.{guess,sub}` available":
(https://github.com/bitcoin/bitcoin/pull/26422#issuecomment-1589094552)
fwiw: This broke the `autogen.sh` part of my development builds on NixOS - found this PR by bisecting. The error is:
```
$ ./autogen.sh
libtoolize: putting auxiliary files in AC_CONFIG_AUX_DIR, 'build-aux'.
libtoolize: copying file 'build-aux/ltmain.sh'
libtoolize: putting macros in AC_CONFIG_MACRO_DIRS, 'build-aux/m4'.
libtoolize: copying file 'build-aux/m4/libtool.m4'
libtoolize: copying file 'build-aux/m4/ltoptions.m4'
libtoolize: copying file 'build-aux/m4/ltsugar.m4'
libtoolize:
...
(https://github.com/bitcoin/bitcoin/pull/26422#issuecomment-1589094552)
fwiw: This broke the `autogen.sh` part of my development builds on NixOS - found this PR by bisecting. The error is:
```
$ ./autogen.sh
libtoolize: putting auxiliary files in AC_CONFIG_AUX_DIR, 'build-aux'.
libtoolize: copying file 'build-aux/ltmain.sh'
libtoolize: putting macros in AC_CONFIG_MACRO_DIRS, 'build-aux/m4'.
libtoolize: copying file 'build-aux/m4/libtool.m4'
libtoolize: copying file 'build-aux/m4/ltoptions.m4'
libtoolize: copying file 'build-aux/m4/ltsugar.m4'
libtoolize:
...
💬 MarcoFalke commented on pull request "build: suppress external warnings by default":
(https://github.com/bitcoin/bitcoin/pull/27872#issuecomment-1589095764)
I think there is still a benefit to developers being notified about warnings in headers. Otherwise no one may notice the false warnings and report them upstream. Also, if they point to real problems, it will be harder to track them down.
(https://github.com/bitcoin/bitcoin/pull/27872#issuecomment-1589095764)
I think there is still a benefit to developers being notified about warnings in headers. Otherwise no one may notice the false warnings and report them upstream. Also, if they point to real problems, it will be harder to track them down.
💬 MarcoFalke commented on pull request "lint: suppress pip spam":
(https://github.com/bitcoin/bitcoin/pull/27871#discussion_r1227961509)
not sure about another place to hardcode the python version, especially if the only reason is to silence a simple single warning.
If you want the warning to be printed less often, you can modify the lines below to be a single call to pip, instead of multiple.
(https://github.com/bitcoin/bitcoin/pull/27871#discussion_r1227961509)
not sure about another place to hardcode the python version, especially if the only reason is to silence a simple single warning.
If you want the warning to be printed less often, you can modify the lines below to be a single call to pip, instead of multiple.
💬 fanquake commented on pull request "build: suppress external warnings by default":
(https://github.com/bitcoin/bitcoin/pull/27872#issuecomment-1589115669)
> I think there is still a benefit to developers being notified about warnings in headers.
They can still be notified, by disabling the suppression.
> Otherwise no one may notice the false warnings and report them upstream.
Then we should stop suppressing them globally in the CI, so we are more likely to see (and then report) the warnings across all relevant platforms / arches. Although then we can't use `-Werror` (which is also why some devs would always be suppressing locally).
(https://github.com/bitcoin/bitcoin/pull/27872#issuecomment-1589115669)
> I think there is still a benefit to developers being notified about warnings in headers.
They can still be notified, by disabling the suppression.
> Otherwise no one may notice the false warnings and report them upstream.
Then we should stop suppressing them globally in the CI, so we are more likely to see (and then report) the warnings across all relevant platforms / arches. Although then we can't use `-Werror` (which is also why some devs would always be suppressing locally).
💬 MarcoFalke commented on pull request "build: suppress external warnings by default":
(https://github.com/bitcoin/bitcoin/pull/27872#issuecomment-1589136728)
Also, it may be good to give some more background. IIRC this is only needed for bdb4 and qt5, so someone compiling bitcoind with sqlite shouldn't have to suppress warnings, no?
(https://github.com/bitcoin/bitcoin/pull/27872#issuecomment-1589136728)
Also, it may be good to give some more background. IIRC this is only needed for bdb4 and qt5, so someone compiling bitcoind with sqlite shouldn't have to suppress warnings, no?
⚠️ 0xB10C opened an issue: "Can't copy to 'build-aux/config.guess' in autoconf.sh: Permission denied"
(https://github.com/bitcoin/bitcoin/issues/27873)
https://github.com/bitcoin/bitcoin/issues/26422 broke the `autogen.sh` part of my development builds on NixOS - found #26422 by bisecting. The error is:
```
$ ./autogen.sh
libtoolize: putting auxiliary files in AC_CONFIG_AUX_DIR, 'build-aux'.
libtoolize: copying file 'build-aux/ltmain.sh'
libtoolize: putting macros in AC_CONFIG_MACRO_DIRS, 'build-aux/m4'.
libtoolize: copying file 'build-aux/m4/libtool.m4'
libtoolize: copying file 'build-aux/m4/ltoptions.m4'
libtoolize: copying file 'bu
...
(https://github.com/bitcoin/bitcoin/issues/27873)
https://github.com/bitcoin/bitcoin/issues/26422 broke the `autogen.sh` part of my development builds on NixOS - found #26422 by bisecting. The error is:
```
$ ./autogen.sh
libtoolize: putting auxiliary files in AC_CONFIG_AUX_DIR, 'build-aux'.
libtoolize: copying file 'build-aux/ltmain.sh'
libtoolize: putting macros in AC_CONFIG_MACRO_DIRS, 'build-aux/m4'.
libtoolize: copying file 'build-aux/m4/libtool.m4'
libtoolize: copying file 'build-aux/m4/ltoptions.m4'
libtoolize: copying file 'bu
...
💬 0xB10C commented on issue "Can't copy to 'build-aux/config.guess' in autoconf.sh: Permission denied":
(https://github.com/bitcoin/bitcoin/issues/27873#issuecomment-1589157862)
https://github.com/bitcoin/bitcoin/blob/8de9bb7a5ab04669369e2bb59ea92a5c1a91a8d2/autogen.sh#L18 evaluates to true for me. The `depends/config.guess` file has a timestamp of `2023-01-01` while the generated `build-aux/config.guess` file has a timestamp of `2021-06-03`.
The generated `build-aux/config.guess` file has the file permissions `-r-xr-xr-x` so overwriting it fails.
```
$ autoreconf --version
autoreconf (GNU Autoconf) 2.71
```
(https://github.com/bitcoin/bitcoin/issues/27873#issuecomment-1589157862)
https://github.com/bitcoin/bitcoin/blob/8de9bb7a5ab04669369e2bb59ea92a5c1a91a8d2/autogen.sh#L18 evaluates to true for me. The `depends/config.guess` file has a timestamp of `2023-01-01` while the generated `build-aux/config.guess` file has a timestamp of `2021-06-03`.
The generated `build-aux/config.guess` file has the file permissions `-r-xr-xr-x` so overwriting it fails.
```
$ autoreconf --version
autoreconf (GNU Autoconf) 2.71
```
💬 0xB10C commented on issue "Can't copy to 'build-aux/config.guess' in autoconf.sh: Permission denied":
(https://github.com/bitcoin/bitcoin/issues/27873#issuecomment-1589198065)
> The generated `build-aux/config.guess` file has the file permissions `-r-xr-xr-x` so overwriting it fails.
`autoreconf --install --force --warnings=all` copies from the nix-store (in this case `/nix/store/3bvdlza5sxy6kjs9y7v0kbx4gxgp0wg8-autoconf-2.71/share/autoconf/build-aux/config.guess`) to `build-aux/config.guess`. The file has `-r-xr-xr-x` permissions in the nix-store. These permissions are preserved when copying. So the problem is a combination of how the nix-store works, how `autorec
...
(https://github.com/bitcoin/bitcoin/issues/27873#issuecomment-1589198065)
> The generated `build-aux/config.guess` file has the file permissions `-r-xr-xr-x` so overwriting it fails.
`autoreconf --install --force --warnings=all` copies from the nix-store (in this case `/nix/store/3bvdlza5sxy6kjs9y7v0kbx4gxgp0wg8-autoconf-2.71/share/autoconf/build-aux/config.guess`) to `build-aux/config.guess`. The file has `-r-xr-xr-x` permissions in the nix-store. These permissions are preserved when copying. So the problem is a combination of how the nix-store works, how `autorec
...
💬 0xB10C commented on issue "Can't copy to 'build-aux/config.guess' in autoconf.sh: Permission denied":
(https://github.com/bitcoin/bitcoin/issues/27873#issuecomment-1589220664)
Changing the permissions for build-aux/config.{guess, sub} with `chmod ug+w` allows us to overwrite the files. Will PR this.
(https://github.com/bitcoin/bitcoin/issues/27873#issuecomment-1589220664)
Changing the permissions for build-aux/config.{guess, sub} with `chmod ug+w` allows us to overwrite the files. Will PR this.
💬 willcl-ark commented on pull request "Mempool: persist mempoolminfee accross restarts":
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1228062756)
nit:
```suggestion
// How often to flush fee estimates to disk.
```
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1228062756)
nit:
```suggestion
// How often to flush fee estimates to disk.
```