💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611456690)
Seems we could factor out `AddressFamilyFromNetwork` at least, this is repeated in literally every implementation 😄
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611456690)
Seems we could factor out `AddressFamilyFromNetwork` at least, this is repeated in literally every implementation 😄
💬 maflcko commented on pull request "fuzz: More accurate coverage reports":
(https://github.com/bitcoin/bitcoin/pull/30156#discussion_r1611462530)
Why not call this at the end of `initialize` inside the function body?
(https://github.com/bitcoin/bitcoin/pull/30156#discussion_r1611462530)
Why not call this at the end of `initialize` inside the function body?
💬 maflcko commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2126828731)
> @ryanofsky If you theory is correct, then having a way to invoke `shrink_to_fit` on `UniValue`'s internal vector might be useful.
Not sure if this is ideal. UniValue is a recursive structure, so calling it on the top level vector only shouldn't cause any difference? Similarly, if shirking is done recursively, the runtime overhead will be equal to that of a copy, so might as well just do a copy instead? Finally, whenever a `UniValue` would be ready to shrink, it is usually one step away from
...
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2126828731)
> @ryanofsky If you theory is correct, then having a way to invoke `shrink_to_fit` on `UniValue`'s internal vector might be useful.
Not sure if this is ideal. UniValue is a recursive structure, so calling it on the top level vector only shouldn't cause any difference? Similarly, if shirking is done recursively, the runtime overhead will be equal to that of a copy, so might as well just do a copy instead? Finally, whenever a `UniValue` would be ready to shrink, it is usually one step away from
...
🤔 glozow reviewed a pull request: "net: add ASMap info in `getrawaddrman` RPC"
(https://github.com/bitcoin/bitcoin/pull/30062#pullrequestreview-2073621757)
ACK 1e54d61c4698debf3329d1960e06078ccbf8063c
(https://github.com/bitcoin/bitcoin/pull/30062#pullrequestreview-2073621757)
ACK 1e54d61c4698debf3329d1960e06078ccbf8063c
💬 glozow commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1611476223)
nit: this could have been a set?
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1611476223)
nit: this could have been a set?
🚀 glozow merged a pull request: "net: add ASMap info in `getrawaddrman` RPC"
(https://github.com/bitcoin/bitcoin/pull/30062)
(https://github.com/bitcoin/bitcoin/pull/30062)
💬 BenWestgate commented on pull request "doc: Update mentions of generating bitcoin.conf":
(https://github.com/bitcoin/bitcoin/pull/30154#issuecomment-2126846372)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30154#issuecomment-2126846372)
Concept ACK
💬 dergoegge commented on pull request "fuzz: More accurate coverage reports":
(https://github.com/bitcoin/bitcoin/pull/30156#discussion_r1611489609)
No reason, done!
(https://github.com/bitcoin/bitcoin/pull/30156#discussion_r1611489609)
No reason, 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_r1611498845)
Oh, I didn't know you could do both. Happy to change if people want, but generally prefer to follow convention of the existing annotations.
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1611498845)
Oh, I didn't know you could do both. Happy to change if people want, but generally prefer to follow convention of the existing annotations.
💬 stickies-v commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1611500389)
I gave the `kernel::Warning` and `node::Warning` enums approach a go because it would be nice to have the possible warnings enumerated, and it ended up being less overhead than I expected. I'm leaning towards updating this PR with that approach. What do you think?
https://github.com/stickies-v/bitcoin/commit/e4ea7aba9f1426dc510dbdbff19788919bf9d3fb - PoC but compiles and tests pass, needs cleanup and doc updates etc
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1611500389)
I gave the `kernel::Warning` and `node::Warning` enums approach a go because it would be nice to have the possible warnings enumerated, and it ended up being less overhead than I expected. I'm leaning towards updating this PR with that approach. What do you think?
https://github.com/stickies-v/bitcoin/commit/e4ea7aba9f1426dc510dbdbff19788919bf9d3fb - PoC but compiles and tests pass, needs cleanup and doc updates etc
👍 BenWestgate approved a pull request: "Drop -dbcache limit"
(https://github.com/bitcoin/bitcoin/pull/28358#pullrequestreview-1601764043)
crACK
(https://github.com/bitcoin/bitcoin/pull/28358#pullrequestreview-1601764043)
crACK
💬 BenWestgate commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#discussion_r1309545616)
I would prefer it quietly round down to the maximum. Especially since it rounds up to the minimum.
I know a few scripts people use that set dbcache to a percentage of available memory that this part of the PR would break for 64GB RAM.
What problem is failing to start meant to solve?
(https://github.com/bitcoin/bitcoin/pull/28358#discussion_r1309545616)
I would prefer it quietly round down to the maximum. Especially since it rounds up to the minimum.
I know a few scripts people use that set dbcache to a percentage of available memory that this part of the PR would break for 64GB RAM.
What problem is failing to start meant to solve?
💬 epiccurious commented on pull request "doc: Update mentions of generating bitcoin.conf":
(https://github.com/bitcoin/bitcoin/pull/30154#discussion_r1611518035)
Good point. My intention here is to reduce ambiguity by being more explicit to the user.
One solution would be to add a check to ensure this section mentioned actually exists in the markdown, but it's probably not worth the added code complexity for little benefit.
Do you prefer to revert this change back?
(https://github.com/bitcoin/bitcoin/pull/30154#discussion_r1611518035)
Good point. My intention here is to reduce ambiguity by being more explicit to the user.
One solution would be to add a check to ensure this section mentioned actually exists in the markdown, but it's probably not worth the added code complexity for little benefit.
Do you prefer to revert this change back?
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611522537)
OK, i went for `QueryDefaultGatewayImpl` for now, with an outer function. Naming it different things on different platforms means having to repeat the `#ifdef` forest, which isn't really worth the slight increase in clarity imo.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611522537)
OK, i went for `QueryDefaultGatewayImpl` for now, with an outer function. Naming it different things on different platforms means having to repeat the `#ifdef` forest, which isn't really worth the slight increase in clarity imo.
💬 epiccurious commented on pull request "doc: Update mentions of generating bitcoin.conf":
(https://github.com/bitcoin/bitcoin/pull/30154#issuecomment-2126901520)
> IIUIC, this change affects only people who is getting core from source
Correct as I understand it. Only impacts people using the source, not the binary distributions.
(https://github.com/bitcoin/bitcoin/pull/30154#issuecomment-2126901520)
> IIUIC, this change affects only people who is getting core from source
Correct as I understand it. Only impacts people using the source, not the binary distributions.
💬 ajtowns commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1611532999)
`EXCLUSIVE_LOCKS_REQUIRED(!foo)` is a stronger assertion; it says that if the caller can see the lock, it also has to have the same assertion. `LOCKS_EXCLUDED(foo)` just says you can't `LOCK()` or have `EXCLUSIVE_LOCKS_REQUIRED(foo)` -- so there's nothing prevent the caller's caller from having taken the lock.
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1611532999)
`EXCLUSIVE_LOCKS_REQUIRED(!foo)` is a stronger assertion; it says that if the caller can see the lock, it also has to have the same assertion. `LOCKS_EXCLUDED(foo)` just says you can't `LOCK()` or have `EXCLUSIVE_LOCKS_REQUIRED(foo)` -- so there's nothing prevent the caller's caller from having taken the lock.
📝 ismaelsadeeq opened a pull request: "Fee Estimation via Fee rate Forecasters"
(https://github.com/bitcoin/bitcoin/pull/30157)
### This PR aims at improving Bitcoin Core fee estimation
### The objectives of this improvement are to:
- Reduce overestimation done by the current `CBlockPolicyEstimator`
- Make the fee estimator aware of the state of the mempool, allowing it to respond to changing conditions immediately.
- Empower node users to be self-sovereign and use their node's estimates, as is now, the majority rely on third parties for fee estimation.
- Simplify the process of adding new fee estimation strat
...
(https://github.com/bitcoin/bitcoin/pull/30157)
### This PR aims at improving Bitcoin Core fee estimation
### The objectives of this improvement are to:
- Reduce overestimation done by the current `CBlockPolicyEstimator`
- Make the fee estimator aware of the state of the mempool, allowing it to respond to changing conditions immediately.
- Empower node users to be self-sovereign and use their node's estimates, as is now, the majority rely on third parties for fee estimation.
- Simplify the process of adding new fee estimation strat
...
💬 epiccurious commented on pull request "doc: note that you can assume C++20.":
(https://github.com/bitcoin/bitcoin/pull/30136#issuecomment-2126906154)
Concept ACK f00801c5fc7ed18518de64dea03b96d7585523ab.
(https://github.com/bitcoin/bitcoin/pull/30136#issuecomment-2126906154)
Concept ACK f00801c5fc7ed18518de64dea03b96d7585523ab.
⚠️ kosuodhmwa opened an issue: "Log: "no wallet support compiled in" when i start bitcoind"
(https://github.com/bitcoin/bitcoin/issues/30158)
hi there
How to compile it with wallet support? I would like to use it as a wallet too - so not just as a full node.
Thank you very much for your feedback(s).
Kind regards, Jan
(https://github.com/bitcoin/bitcoin/issues/30158)
hi there
How to compile it with wallet support? I would like to use it as a wallet too - so not just as a full node.
Thank you very much for your feedback(s).
Kind regards, Jan
💬 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