💬 maflcko commented on issue "Log: "no wallet support compiled in" when i start bitcoind":
(https://github.com/bitcoin/bitcoin/issues/30158#issuecomment-2126963695)
```
sudo apt install libsqlite3-dev
```
See https://github.com/bitcoin/bitcoin/blob/master/doc/build-unix.md#linux-distribution-specific-instructions
(https://github.com/bitcoin/bitcoin/issues/30158#issuecomment-2126963695)
```
sudo apt install libsqlite3-dev
```
See https://github.com/bitcoin/bitcoin/blob/master/doc/build-unix.md#linux-distribution-specific-instructions
💬 sdaftuar commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1611599378)
Looks like the commit message for commit 6b3373248656ef45d73262aa6dba3aec5866f7ca should be updated now, sorry!
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1611599378)
Looks like the commit message for commit 6b3373248656ef45d73262aa6dba3aec5866f7ca should be updated now, sorry!
🚀 fanquake merged a pull request: "contrib: Renew Windows code signing certificate"
(https://github.com/bitcoin/bitcoin/pull/30149)
(https://github.com/bitcoin/bitcoin/pull/30149)
💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1611604355)
Oh duh! Swapped out the last commit for just a deletion of `EraseTxNoLock`, now having all functions call `EraseTx` instead.
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1611604355)
Oh duh! Swapped out the last commit for just a deletion of `EraseTxNoLock`, now having all functions call `EraseTx` instead.
💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1611604731)
done
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1611604731)
done
💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1611604912)
fixed
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1611604912)
fixed
💬 TheCharlatan commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1611606129)
Thanks for testing this out! I forgot that the we'd need to reconstruct the versionbit before, sorry about that. I think I like this better. The reason I am a bit hesitant about keying by strings is that it makes them harder to discover for outside users and forces them to use a variable-size data type as a key for mapping them.
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1611606129)
Thanks for testing this out! I forgot that the we'd need to reconstruct the versionbit before, sorry about that. I think I like this better. The reason I am a bit hesitant about keying by strings is that it makes them harder to discover for outside users and forces them to use a variable-size data type as a key for mapping them.
💬 fanquake commented on pull request "contrib: Renew Windows code signing certificate":
(https://github.com/bitcoin/bitcoin/pull/30149#issuecomment-2126990044)
Backported to 27.x in #30092.
(https://github.com/bitcoin/bitcoin/pull/30149#issuecomment-2126990044)
Backported to 27.x in #30092.
🤔 sdaftuar reviewed a pull request: "Drop -dbcache limit"
(https://github.com/bitcoin/bitcoin/pull/28358#pullrequestreview-2073822380)
Concept ACK.
I don't see much of a philosophical difference between having no limit and having a limit of 16GB on systems where a user has less than 16GB actually available... If we're already requiring users to do something smart, we can keep doing that -- and if we are worried about someone using an inappropriate value we can try to solve that separately (maybe by testing that we can allocate that much at startup or something?).
(https://github.com/bitcoin/bitcoin/pull/28358#pullrequestreview-2073822380)
Concept ACK.
I don't see much of a philosophical difference between having no limit and having a limit of 16GB on systems where a user has less than 16GB actually available... If we're already requiring users to do something smart, we can keep doing that -- and if we are worried about someone using an inappropriate value we can try to solve that separately (maybe by testing that we can allocate that much at startup or something?).
💬 sdaftuar commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#discussion_r1611605241)
Just noticed the filename here should be fixed (25358 vs 28358).
(https://github.com/bitcoin/bitcoin/pull/28358#discussion_r1611605241)
Just noticed the filename here should be fixed (25358 vs 28358).
💬 hebasto commented on pull request "fuzz: More accurate coverage reports":
(https://github.com/bitcoin/bitcoin/pull/30156#issuecomment-2126993578)
Concept ACK on improving coverage.
(https://github.com/bitcoin/bitcoin/pull/30156#issuecomment-2126993578)
Concept ACK on improving coverage.
💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1611615809)
Ah that makes sense. I will leave this as is.
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1611615809)
Ah that makes sense. I will leave this as is.
💬 kevkevinpal commented on pull request "doc: Update mentions of generating bitcoin.conf":
(https://github.com/bitcoin/bitcoin/pull/30154#issuecomment-2127000695)
lgtm ACK [42f788d](https://github.com/bitcoin/bitcoin/pull/30154/commits/42f788d071095b62783c53a75f06831855e86bbd)
The `share/examples/bitcoin.conf` files links to `contrib/devtools/README.md` so instead of send the users to a file to link to another file it makes sense just to use that link directly here
(https://github.com/bitcoin/bitcoin/pull/30154#issuecomment-2127000695)
lgtm ACK [42f788d](https://github.com/bitcoin/bitcoin/pull/30154/commits/42f788d071095b62783c53a75f06831855e86bbd)
The `share/examples/bitcoin.conf` files links to `contrib/devtools/README.md` so instead of send the users to a file to link to another file it makes sense just to use that link directly here
💬 laanwj commented on pull request "doc: note that you can assume C++20.":
(https://github.com/bitcoin/bitcoin/pull/30136#discussion_r1611627399)
Sure, i'm fine with wording it in a way that doesn't imply people understand all nuances, say "The code can use the C++20 standard.", but due to the context it should be a statement about the code, not a compiler requirement.
(https://github.com/bitcoin/bitcoin/pull/30136#discussion_r1611627399)
Sure, i'm fine with wording it in a way that doesn't imply people understand all nuances, say "The code can use the C++20 standard.", but due to the context it should be a statement about the code, not a compiler requirement.
💬 edilmedeiros commented on pull request "doc: Update mentions of generating bitcoin.conf":
(https://github.com/bitcoin/bitcoin/pull/30154#discussion_r1611636584)
Yes.
I don't think adding a check just for a marginal document worth the maintenance burden.
(https://github.com/bitcoin/bitcoin/pull/30154#discussion_r1611636584)
Yes.
I don't think adding a check just for a marginal document worth the maintenance burden.
💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1611638058)
touched up the commit message
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1611638058)
touched up the commit message
💬 brunoerg commented on pull request "fuzz: More accurate coverage reports":
(https://github.com/bitcoin/bitcoin/pull/30156#issuecomment-2127045849)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30156#issuecomment-2127045849)
Concept ACK
💬 jadijadi commented on pull request "Showing Local Addresses in Node Window":
(https://github.com/bitcoin-core/gui/pull/626#discussion_r1611661580)
Agree with this. Will change because "LocalAddress" can be confusing in BTC context.
(https://github.com/bitcoin-core/gui/pull/626#discussion_r1611661580)
Agree with this. Will change because "LocalAddress" can be confusing in BTC context.
👍 willcl-ark approved a pull request: "rpc: avoid copying into UniValue"
(https://github.com/bitcoin/bitcoin/pull/30115#pullrequestreview-2073920635)
ACK d7707d9843b03f20d2a8c5a45d7b3db58e169e6f
Whilst I still measure _slightly_ increased resource usage vs master, even after adding some strategic calls to `shrink_to_fit()` on the `Univalue.values` member (which likely didn't work for the reasons @maflcko described above), I am now convinced this is not something to worry about in practice.
I've done more measurements over the last few days, and even when testing codepaths which create very large numbers of `UniValue`s, I only ever see a
...
(https://github.com/bitcoin/bitcoin/pull/30115#pullrequestreview-2073920635)
ACK d7707d9843b03f20d2a8c5a45d7b3db58e169e6f
Whilst I still measure _slightly_ increased resource usage vs master, even after adding some strategic calls to `shrink_to_fit()` on the `Univalue.values` member (which likely didn't work for the reasons @maflcko described above), I am now convinced this is not something to worry about in practice.
I've done more measurements over the last few days, and even when testing codepaths which create very large numbers of `UniValue`s, I only ever see a
...
💬 vasild commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1611668338)
It is best if all users of `AutoFile` explicitly call the `fclose()` method and check that it succeeds (option 3. from PR description). This is not feasible because there are many users of `AutoFile`. However, some of those users already do call the `fclose()` method explicitly. For those users it is easy and I added checks whether `fclose()` succeeds. `DumpMempool()` is such a user and the change is:
```diff
throw std::runtime_error("FileCommit failed");
- file.fclose()
...
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1611668338)
It is best if all users of `AutoFile` explicitly call the `fclose()` method and check that it succeeds (option 3. from PR description). This is not feasible because there are many users of `AutoFile`. However, some of those users already do call the `fclose()` method explicitly. For those users it is easy and I added checks whether `fclose()` succeeds. `DumpMempool()` is such a user and the change is:
```diff
throw std::runtime_error("FileCommit failed");
- file.fclose()
...