💬 achow101 commented on issue "Memory leak loading legacy wallet (BDB 4.8.30)":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1477072935)
I've been able to reproduce the leak that valgrind finds on master. It appears to be related to the `-privdb` option; looking at the BDB source, the problem occurs inside a block of code guarded by a check for the `DB_PRIVATE` flag. Disabling that with `-privdb=0` "resolves" the leak.
The bisect appears to be incorrect as the parent commit shows the same issue for me. I don't think there's really a solution for us other than upgrading BDB or ditching it entirely.
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1477072935)
I've been able to reproduce the leak that valgrind finds on master. It appears to be related to the `-privdb` option; looking at the BDB source, the problem occurs inside a block of code guarded by a check for the `DB_PRIVATE` flag. Disabling that with `-privdb=0` "resolves" the leak.
The bisect appears to be incorrect as the parent commit shows the same issue for me. I don't think there's really a solution for us other than upgrading BDB or ditching it entirely.
👍 theStack approved a pull request: "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`"
(https://github.com/bitcoin/bitcoin/pull/26261)
re-ACK ec28c89ef69cd3fbb1a82785c841d303b2bf4826
(https://github.com/bitcoin/bitcoin/pull/26261)
re-ACK ec28c89ef69cd3fbb1a82785c841d303b2bf4826
💬 jamesob commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1477135406)
Kicked off a bitcoinperf run; will have some results tomorrow.
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1477135406)
Kicked off a bitcoinperf run; will have some results tomorrow.
👍 theStack approved a pull request: "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg"
(https://github.com/bitcoin/bitcoin/pull/27257)
Code-review ACK 6514352829364ab1ccff3a58faf6ca6ef4aafbbb
(https://github.com/bitcoin/bitcoin/pull/27257)
Code-review ACK 6514352829364ab1ccff3a58faf6ca6ef4aafbbb
💬 theStack commented on pull request "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg":
(https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1142807687)
in commit d816bcced0c1afb22f97df4650a35301a25cd9de: nit: since `fMoreWork` is unused above, could reduce it's scope and declare/init it right here:
```suggestion
bool fMoreWork = poll_result->second;
```
(https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1142807687)
in commit d816bcced0c1afb22f97df4650a35301a25cd9de: nit: since `fMoreWork` is unused above, could reduce it's scope and declare/init it right here:
```suggestion
bool fMoreWork = poll_result->second;
```
💬 instagibbs commented on pull request "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only":
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-1477159508)
Closing for now. We can revisit this at some later date.
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-1477159508)
Closing for now. We can revisit this at some later date.
✅ instagibbs closed a pull request: "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only"
(https://github.com/bitcoin/bitcoin/pull/26398)
(https://github.com/bitcoin/bitcoin/pull/26398)
💬 crystalshay2es commented on pull request "system: allow GUI to initialize default data dir":
(https://github.com/bitcoin/bitcoin/pull/27273#issuecomment-1477197720)
> Default
(https://github.com/bitcoin/bitcoin/pull/27273#issuecomment-1477197720)
> Default
💬 dhruv commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1142867521)
I think I forgot to remove this previously used code. Thanks for pointing it out.
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1142867521)
I think I forgot to remove this previously used code. Thanks for pointing it out.
💬 dhruv commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1142867917)
Done. Although I did leave it as-is in the second commit and addressed it in the fourth commit because I was getting a linker error for multiple definitions trying it on the second commit. The net change isn't different however.
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1142867917)
Done. Although I did leave it as-is in the second commit and addressed it in the fourth commit because I was getting a linker error for multiple definitions trying it on the second commit. The net change isn't different however.
💬 dhruv commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#issuecomment-1477247368)
Addressed review by @theStack.
(https://github.com/bitcoin/bitcoin/pull/25361#issuecomment-1477247368)
Addressed review by @theStack.
💬 dhruv commented on pull request "BIP324: Add encrypted p2p transport {de}serializer":
(https://github.com/bitcoin/bitcoin/pull/23233#issuecomment-1477250064)
Latest push:
- Upstream changes from #25361
- Rebased
(https://github.com/bitcoin/bitcoin/pull/23233#issuecomment-1477250064)
Latest push:
- Upstream changes from #25361
- Rebased
💬 dhruv commented on pull request "BIP324: CKey encode/decode to elligator-swift":
(https://github.com/bitcoin/bitcoin/pull/23432#issuecomment-1477250673)
Rebased.
(https://github.com/bitcoin/bitcoin/pull/23432#issuecomment-1477250673)
Rebased.
💬 dhruv commented on pull request "BIP324: Handshake prerequisites":
(https://github.com/bitcoin/bitcoin/pull/23561#issuecomment-1477251465)
Rebased.
(https://github.com/bitcoin/bitcoin/pull/23561#issuecomment-1477251465)
Rebased.
💬 dhruv commented on pull request "BIP324: Enable v2 P2P encrypted transport":
(https://github.com/bitcoin/bitcoin/pull/24545#issuecomment-1477252274)
Rebased.
(https://github.com/bitcoin/bitcoin/pull/24545#issuecomment-1477252274)
Rebased.
💬 MarcoFalke commented on issue "test: `wallet_importdescriptors.py --descriptors` failure under `--valgrind` ":
(https://github.com/bitcoin/bitcoin/issues/27282#issuecomment-1477374232)
Can remove `--valgrind` from title? See also https://github.com/bitcoin/bitcoin/pull/27199#pullrequestreview-1349425797
(https://github.com/bitcoin/bitcoin/issues/27282#issuecomment-1477374232)
Can remove `--valgrind` from title? See also https://github.com/bitcoin/bitcoin/pull/27199#pullrequestreview-1349425797
💬 MarcoFalke commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1477392747)
> How can we expect the Bitcoin Core wallet to behave reasonably if it can't predict the fees well?
The Bitcoin Core wallet does not use this feature?
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1477392747)
> How can we expect the Bitcoin Core wallet to behave reasonably if it can't predict the fees well?
The Bitcoin Core wallet does not use this feature?
💬 jonasschnelli commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1477425501)
The original idea of this patch was to add a small and easy to maintain **efficient** way to generate a fee histogram to bitcoin core (see original PR description https://github.com/bitcoin/bitcoin/pull/15836#issue-434109585).
As described there, it was always possible to use `getrawmempool` (@jhoenicke script) to achieve this goal.
However, it is **not efficient** to dumb out all transactions of the complete mempool in a json format just to calculate the fee histogram with a python script
...
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1477425501)
The original idea of this patch was to add a small and easy to maintain **efficient** way to generate a fee histogram to bitcoin core (see original PR description https://github.com/bitcoin/bitcoin/pull/15836#issue-434109585).
As described there, it was always possible to use `getrawmempool` (@jhoenicke script) to achieve this goal.
However, it is **not efficient** to dumb out all transactions of the complete mempool in a json format just to calculate the fee histogram with a python script
...
💬 S3RK commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#issuecomment-1477439839)
As far as I understand after this PR is merged we will start unsetting the blank flag and loosing the users' intent.
Are we okay with loosing the intent for the wallets that used the code between this and the follow up PR?
(https://github.com/bitcoin/bitcoin/pull/25634#issuecomment-1477439839)
As far as I understand after this PR is merged we will start unsetting the blank flag and loosing the users' intent.
Are we okay with loosing the intent for the wallets that used the code between this and the follow up PR?
💬 MarcoFalke commented on issue "test: `wallet_importdescriptors.py --descriptors` failure under `--valgrind` ":
(https://github.com/bitcoin/bitcoin/issues/27282#issuecomment-1477448564)
Looks like there are several issues here. The first one, unrelated to the test failure is that the return value is thrown away (thread.join() returns None, always)
(https://github.com/bitcoin/bitcoin/issues/27282#issuecomment-1477448564)
Looks like there are several issues here. The first one, unrelated to the test failure is that the return value is thrown away (thread.join() returns None, always)