🕺 restarted server sorry

💬 achow101 commented on pull request "wallet: return error msg for "too-long-mempool-chain"":
(https://github.com/bitcoin/bitcoin/pull/24845#issuecomment-1474452399)
ACK f3221d373a8623fe4808e00c7a92fbb6e0d6419b

💬 furszy commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1140734813)
nit:
double unneeded `()`

💬 furszy commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1140761441)
There still is a problem with this approach when the user sets a value for the single output.

Basically, the user provided value is being discarded. We don't check if it's within the inputs amount bounds nor if need to create a change output for the remaining amount (value can be lower than the inputs amount).

Current code instead of create a change output when the value is lower than the inputs amount, it sets the value equal to inputs total minus new fee. We should code a test for it too
...

👍 furszy approved a pull request: "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction"
(https://github.com/bitcoin/bitcoin/pull/25273)
diff re-ACK 35fe41f

💬 achow101 commented on pull request "refactor: wallet, do not translate init options names":
(https://github.com/bitcoin/bitcoin/pull/25666#issuecomment-1474477293)
ACK e43a547a3674a31504a60ede9b4912e014a54139

💬 achow101 commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1140811067)
Ah, I see. Actually I've decided to just rebase this PR on top of #27195.

💬 achow101 commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1140812610)
Fixed.

💬 achow101 commented on pull request "bumpfee: allow send coins back to yourself":
(https://github.com/bitcoin/bitcoin/pull/27195#discussion_r1140814055)
In 7cb4dc157a711210e18f3c9b492150b6eb984b30 "bumpfee: enable send coins back to yourself"

`output_value` is not guaranteed to match what the user had actually specified for the single output.

💬 furszy commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1140814808)
Could:
```c++
auto [it, inserted] = m_records.emplace(key_data, value_data);
if (!inserted && overwrite) { // overwrite if requested
it->second = value_data;
inserted = true;
}
return inserted;
```

💬 furszy commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1140822051)
nit:
could use the previously introduced function.
```c++
GetMockableDatabase(wallet).m_pass = false;
```

💬 furszy commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1140836779)
tiny nit:
```c++
return std::make_unique<MockableDatabase>(dynamic_cast<MockableDatabase&>(database).m_records);
```

💬 furszy commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1140832310)
this include doesn't seems to be needed here.

💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1140873089)
maybe I was thinking of `BOOST_CHECK(!addrman->Select(/*new_only=*/true, NET_I2P).first.IsValid());`?

not sure, I [added](https://github.com/bitcoin/bitcoin/compare/09d514583f15860f3bc7ae0c89e640c94fae3c71..17e705428ddf80c7a7f31fe5430d966cf08a37d6#diff-34d1a0e093152df355fc3a6b5b06156f7a9b936bfffb26bb221e62828e44532fR211) `BOOST_CHECK(!addrman->Select(/*new_only=*/true, NET_CJDNS).first.IsValid());` for real this time 🙃

💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1140873566)
opted for the simple assertions because its less invasive & I didn't feel like there was a big advantage of switching over

💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#issuecomment-1474557343)
two small updates:
- added assertions for bounds checking in `GetEntry`
- added the suggested test

💬 amitiuttarwar commented on pull request "p2p: Improve diversification of new connections":
(https://github.com/bitcoin/bitcoin/pull/27264#issuecomment-1474717814)
code review ACK 72e8ffd7f8dbf908e65da6d012ede914596737ab

👍 izlan90 approved a pull request: "p2p: Improve diversification of new connections"
(https://github.com/bitcoin/bitcoin/pull/27264)

💬 martinus commented on pull request "bench: update logging benchmarks":
(https://github.com/bitcoin/bitcoin/pull/26957#discussion_r1140953637)
Benchmarks are registered into a `std::map` so they are alphabetically sorted anyways https://github.com/bitcoin/bitcoin/blob/master/src/bench/bench.h#L68

💬 martinus commented on pull request "bench: update logging benchmarks":
(https://github.com/bitcoin/bitcoin/pull/26957#issuecomment-1474761651)
code review & tested ACK, here are my benchmark results:

| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 3,822.12 | 261,634.61 | 0.3% | 22,931.00 | 8,536.02 | 2.686 | 4,512.00 | 0.1% | 0.5
...