Bitcoin Core Github
44 subscribers
121K links
Download Telegram
💬 ryanofsky commented on pull request "rpc: add path to gethdkey":
(https://github.com/bitcoin/bitcoin/pull/22341#issuecomment-1868311232)
> Initially I'll make a separate PR. If #26728 is closed in favor of the new approach in #29130 then this PR can be closed too.

It seems like you could just rebase this on top of #29130, since the description of the PR and commits aren't going to change that much, they will just be based on #29130 instead of #26728. Either way seems fine, though.
⚠️ GregTonoski opened an issue: "./bitcoin.conf file should not cause confusion with ./datadir/bitcoin.conf"
(https://github.com/bitcoin/bitcoin/issues/29139)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

There is the bitcoin.conf file in the bitcoin-26.0 directory. It's not obvious that the configuration from the file is not used by Bitcoin Core.

Severity: minor (cosmetic issue).

### Expected behaviour

There isn't the same filename (bitcoin.conf) used for the description (./bitcoin.conf) and actual configuration (./datadir/bitcoin.conf).

### Steps to reproduce

Extract archive/insta
...
🤔 furszy reviewed a pull request: "sqlite: Disallow writing from multiple `SQLiteBatch`s"
(https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1795448339)
For the sake of completeness and to avoid potential issues, have expanded the test coverage to exercise the previously mentioned "concurrent database transactions deadlock" behavior. All yours: https://github.com/furszy/bitcoin-core/commit/a80cc56f0d1bdc82e9458f957db03c08620a6dd7. It verifies that the database handler cannot commit/abort changes occurring in a different database handler.

Also, while was working on this, discovered another deadlocking cause.. test https://github.com/furszy/bit
...
💬 furszy commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1435632201)
In 689ed43c2:

Pulling mistake here. Introduced another `DatabaseBatch`. Just need one.
💬 dergoegge commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1435666435)
This wouldn't trigger the warning if the median is equal to `std::numeric_limits<int64_t>::min()`
📝 luke-jr opened a pull request: "Bugfix: RPC: Check for blank rpcauth on a per-param basis"
(https://github.com/bitcoin/bitcoin/pull/29141)
Without this, a blank rpcauth will error if there is a non-blank following it, and a non-blank rpcauth will get ignored if the last one is blank

Includes test updates to detect issues

(It might make sense to support `-norpcauth`/`-rpcauth=0` to disable all defined `-rpcauth` options, but that isn't currently supported, so out of scope here)
💬 luke-jr commented on pull request "Bugfix: RPC: Check for blank rpcauth on a per-param basis":
(https://github.com/bitcoin/bitcoin/pull/29141#issuecomment-1868353002)
I'm not entirely sure what the expected behaviour here is...
💬 pinheadmz commented on issue "./bitcoin.conf file should not cause confusion with ./datadir/bitcoin.conf":
(https://github.com/bitcoin/bitcoin/issues/29139#issuecomment-1868425688)
Are you referring to the file https://github.com/bitcoin/bitcoin/blob/26.x/share/examples/bitcoin.conf ?

Which includes the preface:

```
## All possible configuration options are provided. To use, copy this file
## to your data directory (default or specified by -datadir), uncomment
## options you would like to change, and save the file.
```

`bitcoin.conf` usage is also covered in https://github.com/bitcoin/bitcoin/blob/master/doc/bitcoin-conf.md and again in https://github.com/bitc
...
💬 kashifs commented on pull request "Add Gutter Guard Selector":
(https://github.com/bitcoin/bitcoin/pull/28977#issuecomment-1868501864)
tested ACK [4a94fc8](https://github.com/bitcoin/bitcoin/pull/28977/commits/4a94fc8d827499d120d38002406da8f6c6c731ac)

I read through the conversation, understood the problem and use case for this solution, compiled from source, wrote tests similar to those included
💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1435835891)
more-pythonic-nit:
```suggestion
kwargs['services'] = kwargs.get('services', P2P_SERVICES) | NODE_P2P_V2
```
(also in the `add_outbound_p2p_connection` method below)
👍 theStack approved a pull request: "test/BIP324: functional tests for v2 P2P encryption"
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1795584001)
ACK ad0ae3d2128d04ff4f62a4bf612286d153dc314b
💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1435840695)
nit: IIUC, `b""` is also returned if the decrypted package content is empty. As call-sites currently use
the length integer return value (0) to check for the "only part of packet is received" condition, this isn't a problem, but maybe the description can be improved to avoid confusion.
💬 1440000bytes commented on pull request "Compressed Bitcoin Transactions":
(https://github.com/bitcoin/bitcoin/pull/29134#issuecomment-1868547068)
Concept ACK

25-50% savings look good and compressed raw transactions can help in some cases as mentioned in the PR description.

Related Q&A: https://bitcoin.stackexchange.com/questions/98886/compress-transaction-hex-string (still wont fit in one text message but will need less text messages)
🤔 furszy reviewed a pull request: "p2p: make block download logic aware of limited peers threshold"
(https://github.com/bitcoin/bitcoin/pull/28120#pullrequestreview-1795601906)
Sorry for the delay @naumenkogs. 've been on #28170 before returning here. All good now, it is ready to go.

> The code looks alright, but i'd still rather have [this](https://github.com/bitcoin/bitcoin/pull/28120#pullrequestreview-1557211930) addressed first. It looks like even #28170 doesn't directly handle this, but rather prepare the codebase for another PR handling it?

The concern was:
All connections are from limited peers, we are lagging behind and we no longer disconnect from them
...
💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1435850669)
Thanks updated
💬 fjahr commented on pull request "refactor: C++20: Use std::rotl":
(https://github.com/bitcoin/bitcoin/pull/29085#issuecomment-1868564680)
> If we are going to change it here, should also switch in `hash.cpp`

Done, thanks! I grepped for it didn't find this one, probably because of the all-caps.
💬 GregTonoski commented on issue "./bitcoin.conf file should not cause confusion with ./datadir/bitcoin.conf":
(https://github.com/bitcoin/bitcoin/issues/29139#issuecomment-1868893820)
Yes, the exact prefix in the file is:
```
##
## bitcoin.conf configuration file.
## Generated by contrib/devtools/gen-bitcoin-conf.sh.
##
## Lines beginning with # are comments.
## All possible configuration options are provided. To use, copy this file
## to your data directory (default or specified by -datadir), uncomment
## options you would like to change, and save the file.
##
```

I would suggest renaming the file, e.g. "example-bitcoin.conf".
💬 dimitaracev commented on issue "ci: failure in `wallet_multiwallet.py --legacy-wallet`":
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-1868965467)
I can take a look at this.
💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1436080062)
>Alternatively, could use util::Result failure values after https://github.com/bitcoin/bitcoin/pull/25665 is merged.

will resolve this since I am now using `TxValidationState`