✅ maflcko closed an issue: "Memory leak via API listtransactions"
(https://github.com/bitcoin/bitcoin/issues/25229)
(https://github.com/bitcoin/bitcoin/issues/25229)
💬 maflcko commented on issue "Memory leak via API listtransactions":
(https://github.com/bitcoin/bitcoin/issues/25229#issuecomment-2059651519)
I don't think there is anything that can be done here. The final JSON RPC reply fully written to a single byte buffer must be stored in memory contiguously. If such an amount of memory is not available, it needs to be allocated.
In theory it could be returned, for example by `malloc_trim`, but what would be the point, if on the next RPC call it will need to be allocated again?
(https://github.com/bitcoin/bitcoin/issues/25229#issuecomment-2059651519)
I don't think there is anything that can be done here. The final JSON RPC reply fully written to a single byte buffer must be stored in memory contiguously. If such an amount of memory is not available, it needs to be allocated.
In theory it could be returned, for example by `malloc_trim`, but what would be the point, if on the next RPC call it will need to be allocated again?
💬 maflcko commented on issue "Memory leak via API listtransactions":
(https://github.com/bitcoin/bitcoin/issues/25229#issuecomment-2059656053)
If you are looking for a workaround, you can either provide more memory (or swap), or you could try to call the RPC with smaller `count` values.
(https://github.com/bitcoin/bitcoin/issues/25229#issuecomment-2059656053)
If you are looking for a workaround, you can either provide more memory (or swap), or you could try to call the RPC with smaller `count` values.
💬 achow101 commented on pull request "doc: Add example of mixing private and public keys in descriptors":
(https://github.com/bitcoin/bitcoin/pull/28373#issuecomment-2059662567)
ACK 24b67fa9f602cdeac0e9736256f77d048f616c48
(https://github.com/bitcoin/bitcoin/pull/28373#issuecomment-2059662567)
ACK 24b67fa9f602cdeac0e9736256f77d048f616c48
💬 achow101 commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1566472098)
In 000000000c5fad4aca360a6e914c7779167838db "Add bech32 error short & multiple separators cases"
This is incorrect. `1` is an allowed character in the human readable part, hence a multiple separators check doesn't make sense. The real separator is simply the last `1` that appears. See https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki#bech32
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1566472098)
In 000000000c5fad4aca360a6e914c7779167838db "Add bech32 error short & multiple separators cases"
This is incorrect. `1` is an allowed character in the human readable part, hence a multiple separators check doesn't make sense. The real separator is simply the last `1` that appears. See https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki#bech32
💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1567763258)
suggestion:
call this `med_fee_child` and use the mempoolminfee directly since that's a more meaningful value than `2*FEERATE_1SAT_VB` which is below the floating minfee
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1567763258)
suggestion:
call this `med_fee_child` and use the mempoolminfee directly since that's a more meaningful value than `2*FEERATE_1SAT_VB` which is below the floating minfee
💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567771033)
`header.type` is compared against `BTREE_LEAF` and `BTREE_INTERNAL`, which are the same cases as handled in the `switch()` statement below, which also errors in the default page (though with a slightly different error), so this check is redundant?
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567771033)
`header.type` is compared against `BTREE_LEAF` and `BTREE_INTERNAL`, which are the same cases as handled in the `switch()` statement below, which also errors in the default page (though with a slightly different error), so this check is redundant?
💬 achow101 commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1567767610)
In 000000000fbad5312fe6975959b0ab9ca5daeb16 "Add Base58 Address Prefix Decoder"
It seems a bit odd to me to do all of this work to compute these prefixes just for error messaging. ISTM it would be a lot simpler to just hard code them into chainparams anyways. It's not as if the prefixes will ever change, and new ones are unlikely to be added.
Merging this code seems like an additional maintenance burden for very little gain.
Also, if you insist on keeping this code, it should be placed
...
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1567767610)
In 000000000fbad5312fe6975959b0ab9ca5daeb16 "Add Base58 Address Prefix Decoder"
It seems a bit odd to me to do all of this work to compute these prefixes just for error messaging. ISTM it would be a lot simpler to just hard code them into chainparams anyways. It's not as if the prefixes will ever change, and new ones are unlikely to be added.
Merging this code seems like an additional maintenance burden for very little gain.
Also, if you insist on keeping this code, it should be placed
...
💬 pinheadmz commented on pull request "ZMQ: Support UNIX domain sockets":
(https://github.com/bitcoin/bitcoin/pull/27679#issuecomment-2059679632)
Thanks so much for your review @laanwj addressed your comments
(https://github.com/bitcoin/bitcoin/pull/27679#issuecomment-2059679632)
Thanks so much for your review @laanwj addressed your comments
💬 pinheadmz commented on pull request "system: use %LOCALAPPDATA% as default datadir on windows":
(https://github.com/bitcoin/bitcoin/pull/27064#issuecomment-2059694500)
> Could rebase for fresh CI, if still relevant?
rebased on master
(https://github.com/bitcoin/bitcoin/pull/27064#issuecomment-2059694500)
> Could rebase for fresh CI, if still relevant?
rebased on master
💬 ryanofsky commented on pull request "index: race fix, lock cs_main while 'm_synced' is subject to change":
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2059695927)
> Though maybe if the timing is really unlucky you'd get lots of Commit() calls?
That's a good point. If Commit() is slow and it has to be called more than once, it could make index sync slower, even if it blocks validation code less.
There's a choice about what to do when the Commit() call at the end of the sync is slow and a new block is connected while it is executing.
1. Currently, the new block may get ignored and the index could be corrupted.
2. Before 0faafb57f8298547949cbc0044e
...
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2059695927)
> Though maybe if the timing is really unlucky you'd get lots of Commit() calls?
That's a good point. If Commit() is slow and it has to be called more than once, it could make index sync slower, even if it blocks validation code less.
There's a choice about what to do when the Commit() call at the end of the sync is slow and a new block is connected while it is executing.
1. Currently, the new block may get ignored and the index could be corrupted.
2. Before 0faafb57f8298547949cbc0044e
...
💬 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_r1567788078)
I'm not sure I understand. The previous code expected a crash so the test would know exactly when to check for the expected log message. In the new code bitcoind just runs fine so we have to poll the log for the success message.
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1567788078)
I'm not sure I understand. The previous code expected a crash so the test would know exactly when to check for the expected log message. In the new code bitcoind just runs fine so we have to poll the log for the success message.
💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567792369)
Note that the argument to `ignore()` is unsigned (a `size_t`), might want to error out here if somehow `index < pos`.
(same for the occurence in `InternalPage`)
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567792369)
Note that the argument to `ignore()` is unsigned (a `size_t`), might want to error out here if somehow `index < pos`.
(same for the occurence in `InternalPage`)
💬 achow101 commented on pull request "Wallet: Add GetFullBalance(...) which included used balance":
(https://github.com/bitcoin/bitcoin/pull/29062#discussion_r1567793472)
This doesn't need to be a separate function, just have `GetBalance` include `m_mine_used`. It's up to the caller to figure out what to do with that information.
(https://github.com/bitcoin/bitcoin/pull/29062#discussion_r1567793472)
This doesn't need to be a separate function, just have `GetBalance` include `m_mine_used`. It's up to the caller to figure out what to do with that information.
💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567799862)
It looks like `fclose()` does nothing if `db_file.IsNull()` already holds.
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567799862)
It looks like `fclose()` does nothing if `db_file.IsNull()` already holds.
💬 achow101 commented on pull request "tests: improve wallet multisig descriptor test and docs":
(https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1567805211)
In 8a2021c0f4f40f6f8a37dd5619ecfa3f38084679 "tests: assert xpub order in sortedmulti descriptors do not matter"
It's not clear to me what this `random.sample` is supposed to do or how it improves the test.
(https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1567805211)
In 8a2021c0f4f40f6f8a37dd5619ecfa3f38084679 "tests: assert xpub order in sortedmulti descriptors do not matter"
It's not clear to me what this `random.sample` is supposed to do or how it improves the test.
💬 achow101 commented on pull request "tests: improve wallet multisig descriptor test and docs":
(https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1567795960)
In 80ad8690e25c1ecab2ab1ff282882d5a6745f496 "tests: improve wallet multisig descriptor test and docs"
I don't think it's necessary to explain where the descriptor came from, just a description of it is fine.
(https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1567795960)
In 80ad8690e25c1ecab2ab1ff282882d5a6745f496 "tests: improve wallet multisig descriptor test and docs"
I don't think it's necessary to explain where the descriptor came from, just a description of it is fine.
💬 maflcko commented on issue "Intermittent issue in test/ipc_tests.cpp Fatal glibc error: pthread_mutex_lock.c:450 (__pthread_mutex_lock_full): assertion failed: e != ESRCH || !robust":
(https://github.com/bitcoin/bitcoin/issues/29889#issuecomment-2059743863)
Actually, I think the logs are complete for the log level `test_suite`. If more details are needed `test_suite` could be changed to `all`.
(https://github.com/bitcoin/bitcoin/issues/29889#issuecomment-2059743863)
Actually, I think the logs are complete for the log level `test_suite`. If more details are needed `test_suite` could be changed to `all`.
💬 maflcko commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1567825773)
`wait_for_debug_log` (the variant of the function that accepts raw bytes) does not have a `sleep`, so it will try to maxx out IO at 100%, no?
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1567825773)
`wait_for_debug_log` (the variant of the function that accepts raw bytes) does not have a `sleep`, so it will try to maxx out IO at 100%, no?
💬 achow101 commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#discussion_r1567828103)
In 858fa78637041ae704005d4b6e564dd8245f4b5d "test: Handle functional test disk-full error"
The `+`s at the beginning of the lines are unecessary.
```suggestion
f"Test execution data left in {tmpdir}.\n"
f"Additional storage is needed to execute testing.")
```
(https://github.com/bitcoin/bitcoin/pull/29335#discussion_r1567828103)
In 858fa78637041ae704005d4b6e564dd8245f4b5d "test: Handle functional test disk-full error"
The `+`s at the beginning of the lines are unecessary.
```suggestion
f"Test execution data left in {tmpdir}.\n"
f"Additional storage is needed to execute testing.")
```