💬 pinheadmz commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147716697)
Good thinking about the portable datdir use case, I think it is ok to allow a bypass. It's a bit funny calling it "warn...=1" though. I understand you mean "warn instead of throw an error" but I almost think something like `-ignoreignoredconf` or `-ignoreextraconf` makes more literal sense.
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147716697)
Good thinking about the portable datdir use case, I think it is ok to allow a bypass. It's a bit funny calling it "warn...=1" though. I understand you mean "warn instead of throw an error" but I almost think something like `-ignoreignoredconf` or `-ignoreextraconf` makes more literal sense.
💬 pinheadmz commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147745998)
clang-format wants this to be a single line but I prefer this style too, especially for these longer return values 🤷♂️
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147745998)
clang-format wants this to be a single line but I prefer this style too, especially for these longer return values 🤷♂️
💬 jonatack commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1147763611)
Since we need to maintain compat with v22-v24 not to break user space, it seems helpful to return both so as not to disrupt signal/context as well for those who have been using -addrinfo (no need to break what wasn't broken), thus the idea in https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1147473618. This would also allow more insight for users as to how much is filtered (and possibly to us regarding its usage in making connections).
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1147763611)
Since we need to maintain compat with v22-v24 not to break user space, it seems helpful to return both so as not to disrupt signal/context as well for those who have been using -addrinfo (no need to break what wasn't broken), thus the idea in https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1147473618. This would also allow more insight for users as to how much is filtered (and possibly to us regarding its usage in making connections).
👍 john-moffett approved a pull request: "Add pool based memory resource"
(https://github.com/bitcoin/bitcoin/pull/25325)
ACK 9f947fc3d4b779f017332135323b34e8f216f613
Verified the changes with `range-diff`:
```diff
1: 45508ec79 ! 1: b8401c328 Add pool based memory resource & allocator
@@ src/support/allocators/pool.h (new)
- * whenever it is accessed, but `m_untouched_memory_end` caches this for clarity and efficiency.
+ * whenever it is accessed, but `m_available_memory_end` caches this for clarity and efficiency.
@@ src/support/allocators/pool.h (new)
- * into m_free_lists.
+
...
(https://github.com/bitcoin/bitcoin/pull/25325)
ACK 9f947fc3d4b779f017332135323b34e8f216f613
Verified the changes with `range-diff`:
```diff
1: 45508ec79 ! 1: b8401c328 Add pool based memory resource & allocator
@@ src/support/allocators/pool.h (new)
- * whenever it is accessed, but `m_untouched_memory_end` caches this for clarity and efficiency.
+ * whenever it is accessed, but `m_available_memory_end` caches this for clarity and efficiency.
@@ src/support/allocators/pool.h (new)
- * into m_free_lists.
+
...
💬 pinheadmz commented on pull request "system: allow GUI to initialize default data dir":
(https://github.com/bitcoin/bitcoin/pull/27273#issuecomment-1483034785)
> * I doubt anybody really needs to configure the GUI to locate `bitcoin.conf` in a non-default second location, and then have that `bitcoin.conf` set a datadir in a third location.
Wasn't that the user story in https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1468646427 ?
Either way I see your point about the compatibility issues.
What actually might be more useful for that user or related cases is the option in the GUI to clear the QSettings and so the user can re-set the
...
(https://github.com/bitcoin/bitcoin/pull/27273#issuecomment-1483034785)
> * I doubt anybody really needs to configure the GUI to locate `bitcoin.conf` in a non-default second location, and then have that `bitcoin.conf` set a datadir in a third location.
Wasn't that the user story in https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1468646427 ?
Either way I see your point about the compatibility issues.
What actually might be more useful for that user or related cases is the option in the GUI to clear the QSettings and so the user can re-set the
...
💬 stickies-v commented on pull request "log: Check that the timestamp string is non-empty to avoid undefined behavior":
(https://github.com/bitcoin/bitcoin/pull/27317#discussion_r1147783536)
Could wrap this in an `Assume()` to both make it clear to the reader that it's not supposed to happen and easier to catch any (very unexpected) bugs without crashing non-development builds?
```suggestion
if (m_log_time_micros && Assume(!strStamped.empty())) {
```
(https://github.com/bitcoin/bitcoin/pull/27317#discussion_r1147783536)
Could wrap this in an `Assume()` to both make it clear to the reader that it's not supposed to happen and easier to catch any (very unexpected) bugs without crashing non-development builds?
```suggestion
if (m_log_time_micros && Assume(!strStamped.empty())) {
```
👍 stickies-v approved a pull request: "log: Check that the timestamp string is non-empty to avoid undefined behavior"
(https://github.com/bitcoin/bitcoin/pull/27317)
ACK 73f4eb511cf80cf52b78627347727ca02774b731
(https://github.com/bitcoin/bitcoin/pull/27317)
ACK 73f4eb511cf80cf52b78627347727ca02774b731
💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147772208)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147719940
> Is it possible for you to cache the config file path at the system level like I did [here](https://github.com/bitcoin/bitcoin/pull/27303/files#diff-2babd91c181a89aaeb4a2bd610c8b6bcab0f0b4f5dd20251a3ad0e1742b56380)? That would also fix the confusing log statement produced by the same datadir/conf configuration.
Yes, I was planning to review #27303 today, but I did look at it previously and if #27303 was merged first
...
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147772208)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147719940
> Is it possible for you to cache the config file path at the system level like I did [here](https://github.com/bitcoin/bitcoin/pull/27303/files#diff-2babd91c181a89aaeb4a2bd610c8b6bcab0f0b4f5dd20251a3ad0e1742b56380)? That would also fix the confusing log statement produced by the same datadir/conf configuration.
Yes, I was planning to review #27303 today, but I did look at it previously and if #27303 was merged first
...
💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147783386)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147740130
> Why do you need this os-specific path generator instead of just adding a single subdir to the direcotry where the test framework is already working? On my system at least, it doesn't actually return the "real" default path:
>
>
> `/tmp/bitcoin_func_test_yg47qoyn/home/Library/Application Support/Bitcoin`
IIUC, that path is literally the path this function is generating and returning. The point of this function is
...
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147783386)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147740130
> Why do you need this os-specific path generator instead of just adding a single subdir to the direcotry where the test framework is already working? On my system at least, it doesn't actually return the "real" default path:
>
>
> `/tmp/bitcoin_func_test_yg47qoyn/home/Library/Application Support/Bitcoin`
IIUC, that path is literally the path this function is generating and returning. The point of this function is
...
💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147784827)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147716697
Another name could be allowignoredconf if that is better
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147784827)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147716697
Another name could be allowignoredconf if that is better
💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147775298)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147745998
> clang-format wants this to be a single line but I prefer this style too, especially for these longer return values man_shrugging
Right this was done manually, but you can set a width preference which will cause clang to wrap in the same way, I believe. Have done that for previous PRs.
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147775298)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147745998
> clang-format wants this to be a single line but I prefer this style too, especially for these longer return values man_shrugging
Right this was done manually, but you can set a width preference which will cause clang to wrap in the same way, I believe. Have done that for previous PRs.
💬 willcl-ark commented on issue "Cannot sync with limited RAM and Swap":
(https://github.com/bitcoin/bitcoin/issues/24700#issuecomment-1483063649)
@BebeSparkelSparkel is this still an issue for you with 24.0.1?
I just tried to replicate with a Lubuntu VM using 1GB RAM and the headers all synced fine a number of times in a row. I set my machine up as close as I thought I could to yours with:
```bash
$ sudo swapoff -a
$ grep MemTotal /proc/meminfo
MemTotal: 992260 kB
$ wget https://bitcoincore.org/bin/bitcoin-core-24.0.1/bitcoin-24.0.1-x86_64-linux-gnu.tar.gz
$ tar -xvzf bitcoin-core-24.0.1
$ ./bitcoin-core-24.0.1/bin/bitcoind
...
(https://github.com/bitcoin/bitcoin/issues/24700#issuecomment-1483063649)
@BebeSparkelSparkel is this still an issue for you with 24.0.1?
I just tried to replicate with a Lubuntu VM using 1GB RAM and the headers all synced fine a number of times in a row. I set my machine up as close as I thought I could to yours with:
```bash
$ sudo swapoff -a
$ grep MemTotal /proc/meminfo
MemTotal: 992260 kB
$ wget https://bitcoincore.org/bin/bitcoin-core-24.0.1/bitcoin-24.0.1-x86_64-linux-gnu.tar.gz
$ tar -xvzf bitcoin-core-24.0.1
$ ./bitcoin-core-24.0.1/bin/bitcoind
...
💬 ryanofsky commented on pull request "system: allow GUI to initialize default data dir":
(https://github.com/bitcoin/bitcoin/pull/27273#issuecomment-1483067013)
> > * I doubt anybody really needs to configure the GUI to locate `bitcoin.conf` in a non-default second location, and then have that `bitcoin.conf` set a datadir in a third location.
>
> Wasn't that the user story in [#27246 (comment)](https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1468646427) ?
Yes but I think a better alternative for that user would be to use -includeconf which works today (or symlinks) rather than datadir daisy chaining which doesn't work and would need t
...
(https://github.com/bitcoin/bitcoin/pull/27273#issuecomment-1483067013)
> > * I doubt anybody really needs to configure the GUI to locate `bitcoin.conf` in a non-default second location, and then have that `bitcoin.conf` set a datadir in a third location.
>
> Wasn't that the user story in [#27246 (comment)](https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1468646427) ?
Yes but I think a better alternative for that user would be to use -includeconf which works today (or symlinks) rather than datadir daisy chaining which doesn't work and would need t
...
💬 MarcoFalke commented on issue "Cannot sync with limited RAM and Swap":
(https://github.com/bitcoin/bitcoin/issues/24700#issuecomment-1483100077)
To rule out that this was caused by bitcoin-qt, @BebeSparkelSparkel could also try to run with just bitcoind?
(https://github.com/bitcoin/bitcoin/issues/24700#issuecomment-1483100077)
To rule out that this was caused by bitcoin-qt, @BebeSparkelSparkel could also try to run with just bitcoind?
💬 willcl-ark commented on issue "Cannot sync with limited RAM and Swap":
(https://github.com/bitcoin/bitcoin/issues/24700#issuecomment-1483102824)
Ah good catch, I missed he was running `bitcoin-qt` (and foolishly _assumed_ on a low powered device like that it would be `bitcoind`)... In the meantime I can re-test with `bitcoin-qt`.
(https://github.com/bitcoin/bitcoin/issues/24700#issuecomment-1483102824)
Ah good catch, I missed he was running `bitcoin-qt` (and foolishly _assumed_ on a low powered device like that it would be `bitcoind`)... In the meantime I can re-test with `bitcoin-qt`.
💬 willcl-ark commented on issue "Cannot sync with limited RAM and Swap":
(https://github.com/bitcoin/bitcoin/issues/24700#issuecomment-1483113206)
Update: `bitcoin-qt` has passed headers sync and entered into IBD on the same 1GB machine with swap disabled.
I think therefore unless @BebeSparkelSparkel confirms this is still an issue for him on his hardware we can close this one out soon.
(https://github.com/bitcoin/bitcoin/issues/24700#issuecomment-1483113206)
Update: `bitcoin-qt` has passed headers sync and entered into IBD on the same 1GB machine with swap disabled.
I think therefore unless @BebeSparkelSparkel confirms this is still an issue for him on his hardware we can close this one out soon.
📝 theStack opened a pull request: "test: various `converttopsbt` check cleanups in rpc_psbt.py"
(https://github.com/bitcoin/bitcoin/pull/27325)
In the functional test rpc_psbt.py, some comments around the `converttopsbt` RPC checks are wrong or outdated and can be removed:
> _Error could be either "TX decode failed" (segwit inputs causes
> parsing to fail) or "Inputs must not have scriptSigs and
> scriptWitnesses"_
Decoding a valid TX with at least one input always succeeds with the [heuristic](https://github.com/bitcoin/bitcoin/blob/e352f5ab6b60ec1cc549997275e945238508cdee/src/core_read.cpp#L126), i.e. this comment is not right
...
(https://github.com/bitcoin/bitcoin/pull/27325)
In the functional test rpc_psbt.py, some comments around the `converttopsbt` RPC checks are wrong or outdated and can be removed:
> _Error could be either "TX decode failed" (segwit inputs causes
> parsing to fail) or "Inputs must not have scriptSigs and
> scriptWitnesses"_
Decoding a valid TX with at least one input always succeeds with the [heuristic](https://github.com/bitcoin/bitcoin/blob/e352f5ab6b60ec1cc549997275e945238508cdee/src/core_read.cpp#L126), i.e. this comment is not right
...
💬 MarcoFalke commented on issue "Cannot sync with limited RAM and Swap":
(https://github.com/bitcoin/bitcoin/issues/24700#issuecomment-1483122740)
Also, @BebeSparkelSparkel was running the debian package, which is compiled differently. And on arm64, not x86.
But yeah closing for now. Please let us know if this is still an issue or if this should be re-opened.
If you still have the arm machine running, it would be good to re-check with the binaries from https://bitcoincore.org/en/download/
(https://github.com/bitcoin/bitcoin/issues/24700#issuecomment-1483122740)
Also, @BebeSparkelSparkel was running the debian package, which is compiled differently. And on arm64, not x86.
But yeah closing for now. Please let us know if this is still an issue or if this should be re-opened.
If you still have the arm machine running, it would be good to re-check with the binaries from https://bitcoincore.org/en/download/
✅ MarcoFalke closed an issue: "Cannot sync with limited RAM and Swap"
(https://github.com/bitcoin/bitcoin/issues/24700)
(https://github.com/bitcoin/bitcoin/issues/24700)
💬 pinheadmz commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147851308)
Ok I understand now, I tried removing it but now I see the environment variables are important for setting the initial "where's the conf" directory without spoiling `-datadir`. I also notice `get_temp_default_datadir()` has a windows branch but windows will skip this test anyway! Not a bad idea to leave it in there anyway though I guess.
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147851308)
Ok I understand now, I tried removing it but now I see the environment variables are important for setting the initial "where's the conf" directory without spoiling `-datadir`. I also notice `get_temp_default_datadir()` has a windows branch but windows will skip this test anyway! Not a bad idea to leave it in there anyway though I guess.