💬 Sjors commented on pull request "Use Txid in COutpoint":
(https://github.com/bitcoin/bitcoin/pull/28922#issuecomment-1820940555)
@maflcko perhaps we should add another `RPCArg` type that's 32 characters `STR_HEX`?

💬 dergoegge commented on pull request "Use Txid in COutpoint":
(https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1400613097)
`Txid::begin` etc. return `std::byte*` which can't be used for arithmetic operations.

See https://en.cppreference.com/w/cpp/types/byte.

💬 dergoegge commented on pull request "Use Txid in COutpoint":
(https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1400615735)
Looks like `ParseHashO` has a length check internally?

💬 dergoegge commented on pull request "Use Txid in COutpoint":
(https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1400618039)
Not sure what you mean here? `entry.first` is a `uint256`.

💬 Sjors commented on pull request "Use Txid in COutpoint":
(https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1400623243)
I was confused where `Txid` comes to play here. But I see `key` is a `COutPoint`.

💬 Sjors commented on pull request "Use Txid in COutpoint":
(https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1400626737)
I think I copy-pasted this comment in the wrong file. Can be ignored

💬 Sjors commented on pull request "Use Txid in COutpoint":
(https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1400632982)
Ah, it calls `ParseHashV` which indeed checks that it's 64 characters. So then only the `rest.cpp` line above doesn't check the length (and it's terribly documented).

💬 Sjors commented on pull request "Use Txid in COutpoint":
(https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1400635225)
Update: it does, via `ParseHashV` - deleting the other comments below

💬 toolsopen commented on issue "sendrawtransaction takes too long":
(https://github.com/bitcoin/bitcoin/issues/28745#issuecomment-1821007250)
> Thanks for the update. Wallet performance is something that's being continually worked on.
>
> Did you end up upgrading versions (too), or still running on V22?

I also updated the final version to 25.1

💬 toolsopen commented on issue "sendrawtransaction takes too long":
(https://github.com/bitcoin/bitcoin/issues/28745#issuecomment-1821012378)
> Assuming this is a bdb wallet (can be checked via `getwalletinfo()[format]`), I don't think it is worth to spend time on optimizing the code, given that it will be removed in the future.

I use it for utxo maintenance and transaction broadcasting, but an overly large wallet will definitely have a great impact on performance.

💬 maflcko commented on issue "sendrawtransaction takes too long":
(https://github.com/bitcoin/bitcoin/issues/28745#issuecomment-1821019293)
What does the `getwalletinfo` RPC return in the `format` field, for you?

👍 theStack approved a pull request: "coins: make sure PoolAllocator uses the correct alignment"
(https://github.com/bitcoin/bitcoin/pull/28913#pullrequestreview-1742181205)
Tested ACK d5b4c0b69e543de51bb37d602d488ee0949ba185

Using [the emulated 32-bit ARM machine using qemu and Debian bookworm for armhf](https://gist.github.com/theStack/6eaeadd3fa1e521ed038b1ed93c101d6),
I could both reproduce issue #28906 on master and verify the fix in this PR. Having 2GB of RAM available without swap memory, bitcoind failed due to OOM on block height 322923 on the master branch, as the coins flush never happened up to that height due to an underestimation of the cache size.
...

💬 ryanofsky commented on pull request "multiprocess: Add basic type conversion hooks":
(https://github.com/bitcoin/bitcoin/pull/28921#discussion_r1400413443)
re: https://github.com/bitcoin/bitcoin/pull/28921#discussion_r1400158381

> The values are ignored either way, so it seems better to remove them. `CDataStream` will go away anyway, soon.

Thanks, will update. This previously was used to serialize transactions, but now with #28438 that is broken anyway, so I'll need to find something different to do for transactions.

💬 ryanofsky commented on pull request "multiprocess: Add basic type conversion hooks":
(https://github.com/bitcoin/bitcoin/pull/28921#discussion_r1400723520)
re: https://github.com/bitcoin/bitcoin/pull/28921#discussion_r1400551468

> This type of interface where we pass serialized objects (using our serialization) via capnp data obviously fits in quite well with our code base, but I am wondering if this is a nice design for external use? since now anybody using it needs capnp and our serialization format.

I think the choice will vary on a case-by-case basis. If you look at [`chain.capnp`](https://github.com/ryanofsky/bitcoin/blob/pr/ipc/src/ipc/
...

💬 ryanofsky commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1400731107)
I'm wondering if TX_WITH_WITNESS calls should be required and specified everywhere (107 places)

If you write `stream << object`, it seems like it would make sense for the object to be fully serialized by default, with an option to partially serialize it. I'm asking in the context of #28921 / #10102, because before this PR, CTransaction objects could be serialized normally like other objects, and now they require a TX_WITH_WITNESS wrapper to have the same behavior. If TX_WITH_WITNESS were just
...

📝 theStack opened a pull request: "script/sign: avoid duplicated signature verification after signing (+introduce signing benchmarks)"
(https://github.com/bitcoin/bitcoin/pull/28923)
This PR is a small performance improvement on the `SignTransaction` function, which is used mostly by the wallet (obviously) and the `signrawtransactionwithkey` RPC. The lower-level function `ProduceSignature` already calls `VerifyScript` internally as last step in order to check whether the signature data is complete:
https://github.com/bitcoin/bitcoin/blob/daa56f7f665183bcce3df146f143be37f33c123e/src/script/sign.cpp#L568-L570

If and only if that is the case, the `complete` field of the `Si
...

💬 glozow commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1400876966)
Could this use the `Wtxid` type?

📝 maflcko opened a pull request: " refactor: Remove unused and fragile string interface from arith_uint256 "
(https://github.com/bitcoin/bitcoin/pull/28924)
The string interface (`base_uint(const std::string&)`, as well as `base_uint::SetHex`) is problematic for many reasons:

* It is unused (except in test-only code).
* It is redundant with the `uint256` string interface: `std::string -> uint256 -> UintToArith256`.
* It is brittle, because it inherits the brittle `uint256` string interface, which is brittle due to the use of `c_str()` (embedded null will be treated as end-of string), etc ...

Instead of fixing the interface, remove it since i
...

💬 mzumsande commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1400824727)
no, I don't know a better approach, but thanks for the explanation!

💬 mzumsande commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1401009634)
The test fails in debug mode because this assume conflicts with the test saying "Adding a duplicate transaction should not happen, but it does happen, nothing should break.".

💬 mzumsande commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1401014445)
Although this reduced it, I think the situation where the child is fanouted ahead could still happen if we receive the parent first, add it to the recon set, and only after that receive the child and decide to fanout it.
Not sure if that is a problem though.